Remove spelling markers in disabled and readonly form elements

Remove spelling markers in disabled and readonly form elements

If you mark as disabled or readonly a form element,
any spelling marker that appears there should disappear.

The spec is pretty clear on this [1]:
  "User agents must only consider the following pieces of text
   as checkable for the purposes of this feature:
   * The value of input elements whose type attributes are in the Text,
     Search, URL, or E-mail states and that are mutable
     (i.e. that do not have the readonly attribute specified
     and that are not disabled).
   * The value of textarea elements that do not have
     a readonly attribute and that are not disabled.
   * ..."

Regarding test fast/css/invalidation/read-only-write-pseudo.html,
it's updated as now updateStyleAndLayoutTreeForNode() is already
called when the markers are removed. So when the test forces it
with updateStyleAndReturnAffectedElementCount() the result is 0.

BUG=687949
TEST=editing/spelling/spellcheck-remove-markers-disabled-readonly.html

[1] https://html.spec.whatwg.org/multipage/interaction.html#spelling-and-grammar-checking
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

[Manuel Rego, 2017-02-02 13:37:18]

rego@igalia.com changed reviewers:
+ tkent@chromium.org, xiaochengh@chromium.org, yosin@chromium.org

[Manuel Rego, 2017-02-02 13:37:19]

Following a similar approach to what was done for "contenteditable":
https://codereview.chromium.org/2650183002/

[tkent, 2017-02-03 00:09:59]

Please address test failures.

https://codereview.chromium.org/2669213002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/2669213002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLFormControlElement.cpp:148: } else if
(params.name == readonlyAttr &&
I think no reason to handle this in HTMLFormControlElement.  Move it to
TextControlElement.

[Xiaocheng, 2017-02-03 04:54:07]

https://codereview.chromium.org/2669213002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/TextControlElement.cpp (right):

https://codereview.chromium.org/2669213002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/TextControlElement.cpp:921: void
TextControlElement::removeSpellingAndGrammarMarkersOnInnerEditor() {
This function seems unnecessary. I think checking isDisabledOrReadOnly()
followed by calling SpellChecker::didEndEditingOnTextField() suffices.

Besides, there should be a nullptr check with document().frame(). A similar
check should also be added to fix crbug.com/687984.

[Manuel Rego, 2017-02-03 14:19:06]

Patchset #2 (id:20001) has been deleted

[Manuel Rego, 2017-02-03 16:27:00]

Description was changed from

==========
Remove spelling markers in disabled and readonly form elements

If you mark as disabled or readonly a form element,
any spelling marker that appears there should disappear.

The spec is pretty clear on this [1]:
  "User agents must only consider the following pieces of text
   as checkable for the purposes of this feature:
   * The value of input elements whose type attributes are in the Text,
     Search, URL, or E-mail states and that are mutable
     (i.e. that do not have the readonly attribute specified
     and that are not disabled).
   * The value of textarea elements that do not have
     a readonly attribute and that are not disabled.
   * ..."

BUG=687949
TEST=editing/spelling/spellcheck-remove-markers-disabled-readonly.html

[1]
https://html.spec.whatwg.org/multipage/interaction.html#spelling-and-grammar-...
==========

to

==========
Remove spelling markers in disabled and readonly form elements

If you mark as disabled or readonly a form element,
any spelling marker that appears there should disappear.

The spec is pretty clear on this [1]:
  "User agents must only consider the following pieces of text
   as checkable for the purposes of this feature:
   * The value of input elements whose type attributes are in the Text,
     Search, URL, or E-mail states and that are mutable
     (i.e. that do not have the readonly attribute specified
     and that are not disabled).
   * The value of textarea elements that do not have
     a readonly attribute and that are not disabled.
   * ..."

Regarding test fast/css/invalidation/read-only-write-pseudo.html,
it's updated as now updateStyleAndLayoutTreeForNode() is already
called when the markers are removed. So when the test forces it
with updateStyleAndReturnAffectedElementCount() the result is 0.

BUG=687949
TEST=editing/spelling/spellcheck-remove-markers-disabled-readonly.html

[1]
https://html.spec.whatwg.org/multipage/interaction.html#spelling-and-grammar-...
==========

[Manuel Rego, 2017-02-06 08:14:57]

Thanks for the reviews, uploaded a new version.

Now everything is implemented on TextControlElement  
and using didEndEditingOnTextField().
> Regarding test fast/css/invalidation/read-only-write-pseudo.html,
> it's updated as now updateStyleAndLayoutTreeForNode() is already
> called when the markers are removed. So when the test forces it
> with updateStyleAndReturnAffectedElementCount() the result is 0.

Please let me know what do you think about it.

[tkent, 2017-02-06 23:28:07]

> Regarding test fast/css/invalidation/read-only-write-pseudo.html,
> it's updated as now updateStyleAndLayoutTreeForNode() is already
> called when the markers are removed. So when the test forces it
> with updateStyleAndReturnAffectedElementCount() the result is 0.

Sounds reasonable.

https://codereview.chromium.org/2669213002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/TextControlElement.h (right):

https://codereview.chromium.org/2669213002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/TextControlElement.h:159: void
attributeChanged(const AttributeModificationParams&) override;
You don't need to override |attributeChanged|. You can add the logic to
|parseAttribute| instead.

[Manuel Rego, 2017-02-07 10:26:25]

Thanks for the review, uploading new version using parseAttribute().

https://codereview.chromium.org/2669213002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/TextControlElement.h (right):

https://codereview.chromium.org/2669213002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/TextControlElement.h:159: void
attributeChanged(const AttributeModificationParams&) override;
On 2017/02/06 23:28:07, tkent wrote:
> You don't need to override |attributeChanged|. You can add the logic to
> |parseAttribute| instead.

Done.

[Xiaocheng, 2017-02-07 11:23:01]

xiaochengh@chromium.org changed reviewers:
- xiaochengh@chromium.org

[Xiaocheng, 2017-02-07 11:23:02]

Removing myself. I can't review in these days.

[tkent, 2017-02-07 14:17:06]

https://codereview.chromium.org/2669213002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/TextControlElement.cpp (right):

https://codereview.chromium.org/2669213002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/TextControlElement.cpp:721:
setNeedsWillValidateCheck();
Why do we need setNeedsWillValidateCheck()?

[Manuel Rego, 2017-02-07 15:06:03]

https://codereview.chromium.org/2669213002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/TextControlElement.cpp (right):

https://codereview.chromium.org/2669213002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/TextControlElement.cpp:721:
setNeedsWillValidateCheck();
On 2017/02/07 14:17:06, tkent wrote:
> Why do we need setNeedsWillValidateCheck()?

Now that we use parseAttribute() I need to call setNeedsWillValidateCheck()
or I'll get a crash: http://sprunge.us/dQSE
It seems to be triggered by the call to
updateStyleAndLayoutTreeForNode() in removeSpellingAndGrammarMarkers().

If I use attributeChanged() I don't need.
So I'm not sure if it's better to come back to the previous approach
and override attributeChanged().

[yosin_UTC9, 2017-02-08 02:07:44]

https://codereview.chromium.org/2669213002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/TextControlElement.cpp (right):

https://codereview.chromium.org/2669213002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/TextControlElement.cpp:721:
setNeedsWillValidateCheck();
On 2017/02/07 at 15:06:03, Manuel Rego wrote:
> On 2017/02/07 14:17:06, tkent wrote:
> > Why do we need setNeedsWillValidateCheck()?
> 
> Now that we use parseAttribute() I need to call setNeedsWillValidateCheck()
> or I'll get a crash: http://sprunge.us/dQSE
> It seems to be triggered by the call to
> updateStyleAndLayoutTreeForNode() in removeSpellingAndGrammarMarkers().
> 
> If I use attributeChanged() I don't need.
> So I'm not sure if it's better to come back to the previous approach
> and override attributeChanged().

It seems we should refactor removeSpellingAndGrammarMarkers().

There are two call sites of removeSpellingAndGrammarMarkers()
 1. HTMLElement::attributeChanged() calls removeXXX() with
ElementsType::kOnlyNonEditable
 2. SpellChecker::didEndEditingOnTextField(): calls removeXXX() with
ElementsType::kAll

If we change HTMLElement::attributeChanged() to call removeXXX() only when
contenteditable="false",
we don't need to have ElementType parameter in removeXXX() and we don't need to
call hasEditableStyle() and updateStyleAndLayoutXXX()
in removeSpellingAndGrammarMarkers().


Could you make this change in another CL for a preparation of this patch?

[tkent, 2017-02-08 02:12:14]

https://codereview.chromium.org/2669213002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/TextControlElement.cpp (right):

https://codereview.chromium.org/2669213002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/TextControlElement.cpp:721:
setNeedsWillValidateCheck();
On 2017/02/07 at 15:06:03, Manuel Rego wrote:
> On 2017/02/07 14:17:06, tkent wrote:
> > Why do we need setNeedsWillValidateCheck()?
> 
> Now that we use parseAttribute() I need to call setNeedsWillValidateCheck()
> or I'll get a crash: http://sprunge.us/dQSE
> It seems to be triggered by the call to
> updateStyleAndLayoutTreeForNode() in removeSpellingAndGrammarMarkers().
> 
> If I use attributeChanged() I don't need.
> So I'm not sure if it's better to come back to the previous approach
> and override attributeChanged().

Thanks.  I understand.  We handle |disabled| attribute in
HTMLFormControlElement::attributeChanged, not parseAttribute().
With this patch, style is invalidated twice if disabled attribute is changed.
It's not ideal.
I'd like:
 * TextControlElement overrides disabledAttributeChanged() like Patch Set 1, and
 * TextControlElement::parseAttribute() handles readonlyAttr.

[Manuel Rego, 2017-02-08 09:00:34]

https://codereview.chromium.org/2669213002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/TextControlElement.cpp (right):

https://codereview.chromium.org/2669213002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/TextControlElement.cpp:721:
setNeedsWillValidateCheck();
On 2017/02/08 02:07:44, yosin_BlinkOn_slow wrote:
> On 2017/02/07 at 15:06:03, Manuel Rego wrote:
> > On 2017/02/07 14:17:06, tkent wrote:
> > > Why do we need setNeedsWillValidateCheck()?
> > 
> > Now that we use parseAttribute() I need to call setNeedsWillValidateCheck()
> > or I'll get a crash: http://sprunge.us/dQSE
> > It seems to be triggered by the call to
> > updateStyleAndLayoutTreeForNode() in removeSpellingAndGrammarMarkers().
> > 
> > If I use attributeChanged() I don't need.
> > So I'm not sure if it's better to come back to the previous approach
> > and override attributeChanged().
> 
> It seems we should refactor removeSpellingAndGrammarMarkers().
> 
> There are two call sites of removeSpellingAndGrammarMarkers()
>  1. HTMLElement::attributeChanged() calls removeXXX() with
> ElementsType::kOnlyNonEditable
>  2. SpellChecker::didEndEditingOnTextField(): calls removeXXX() with
> ElementsType::kAll
> 
> If we change HTMLElement::attributeChanged() to call removeXXX() only when
> contenteditable="false",
> we don't need to have ElementType parameter in removeXXX() and we don't need
to
> call hasEditableStyle() and updateStyleAndLayoutXXX()
> in removeSpellingAndGrammarMarkers().

We still need to use hasEditableStyle() in removeSpellingAndGrammarMarkers().
For example if you have:
  <div id="parent" contenteditable>
    foo
    <span id="child" contenteditable>test</span>
    bar
  </div>

If you mark "parent" as "contenteditable=false",
you have to remove the spell markers from "foo" and "bar",
but not inside "child" as it's still editable.
That was the reason why we added the ElementType parameter in
https://codereview.chromium.org/2650183002/

[yosin_UTC9, 2017-02-08 09:52:02]

On 2017/02/08 at 09:00:34, rego wrote:
>
https://codereview.chromium.org/2669213002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/html/TextControlElement.cpp (right):
> 
>
https://codereview.chromium.org/2669213002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/TextControlElement.cpp:721:
setNeedsWillValidateCheck();
> On 2017/02/08 02:07:44, yosin_BlinkOn_slow wrote:
> > On 2017/02/07 at 15:06:03, Manuel Rego wrote:
> > > On 2017/02/07 14:17:06, tkent wrote:
> > > > Why do we need setNeedsWillValidateCheck()?
> > > 
> > > Now that we use parseAttribute() I need to call
setNeedsWillValidateCheck()
> > > or I'll get a crash: http://sprunge.us/dQSE
> > > It seems to be triggered by the call to
> > > updateStyleAndLayoutTreeForNode() in removeSpellingAndGrammarMarkers().
> > > 
> > > If I use attributeChanged() I don't need.
> > > So I'm not sure if it's better to come back to the previous approach
> > > and override attributeChanged().
> > 
> > It seems we should refactor removeSpellingAndGrammarMarkers().
> > 
> > There are two call sites of removeSpellingAndGrammarMarkers()
> >  1. HTMLElement::attributeChanged() calls removeXXX() with
> > ElementsType::kOnlyNonEditable
> >  2. SpellChecker::didEndEditingOnTextField(): calls removeXXX() with
> > ElementsType::kAll
> > 
> > If we change HTMLElement::attributeChanged() to call removeXXX() only when
> > contenteditable="false",
> > we don't need to have ElementType parameter in removeXXX() and we don't need
to
> > call hasEditableStyle() and updateStyleAndLayoutXXX()
> > in removeSpellingAndGrammarMarkers().
> 
> We still need to use hasEditableStyle() in removeSpellingAndGrammarMarkers().
> For example if you have:
>   <div id="parent" contenteditable>
>     foo
>     <span id="child" contenteditable>test</span>
>     bar
>   </div>
> 
> If you mark "parent" as "contenteditable=false",
> you have to remove the spell markers from "foo" and "bar",
> but not inside "child" as it's still editable.
> That was the reason why we added the ElementType parameter in
> https://codereview.chromium.org/2650183002/

It seems it is better to removal of markers in idle task rather than each
attribute change.
We can do:
1. schedule idle task for removing "useless markers"
2. During scanning markers in paint, e.g. InlineTextBoxPainter, check editabilit
and mark useless

This way makes no performance regression, maybe faster? since we don't need to
scan markers.

[yosin_UTC9, 2017-02-22 02:21:08]

yosin@chromium.org changed reviewers:
+ xiaochengh@chromium.org

[Manuel Rego, 2017-03-03 12:40:03]

Description was changed from

==========
Remove spelling markers in disabled and readonly form elements

If you mark as disabled or readonly a form element,
any spelling marker that appears there should disappear.

The spec is pretty clear on this [1]:
  "User agents must only consider the following pieces of text
   as checkable for the purposes of this feature:
   * The value of input elements whose type attributes are in the Text,
     Search, URL, or E-mail states and that are mutable
     (i.e. that do not have the readonly attribute specified
     and that are not disabled).
   * The value of textarea elements that do not have
     a readonly attribute and that are not disabled.
   * ..."

Regarding test fast/css/invalidation/read-only-write-pseudo.html,
it's updated as now updateStyleAndLayoutTreeForNode() is already
called when the markers are removed. So when the test forces it
with updateStyleAndReturnAffectedElementCount() the result is 0.

BUG=687949
TEST=editing/spelling/spellcheck-remove-markers-disabled-readonly.html

[1]
https://html.spec.whatwg.org/multipage/interaction.html#spelling-and-grammar-...
==========

to

==========
Remove spelling markers in disabled and readonly form elements

If you mark as disabled or readonly a form element,
any spelling marker that appears there should disappear.

The spec is pretty clear on this [1]:
  "User agents must only consider the following pieces of text
   as checkable for the purposes of this feature:
   * The value of input elements whose type attributes are in the Text,
     Search, URL, or E-mail states and that are mutable
     (i.e. that do not have the readonly attribute specified
     and that are not disabled).
   * The value of textarea elements that do not have
     a readonly attribute and that are not disabled.
   * ..."

Regarding test fast/css/invalidation/read-only-write-pseudo.html,
it's updated as now updateStyleAndLayoutTreeForNode() is already
called when the markers are removed. So when the test forces it
with updateStyleAndReturnAffectedElementCount() the result is 0.

BUG=687949
TEST=editing/spelling/spellcheck-remove-markers-disabled-readonly.html

[1]
https://html.spec.whatwg.org/multipage/interaction.html#spelling-and-grammar-...
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Manuel Rego, 2017-03-03 12:44:06]

Sorry for the delay on coming back to this.

On 2017/02/08 09:52:02, yosin_UTC9 wrote:
> It seems it is better to removal of markers in idle task rather than each
> attribute change.
> We can do:
> 1. schedule idle task for removing "useless markers"
> 2. During scanning markers in paint, e.g. InlineTextBoxPainter, check
editabilit
> and mark useless
> 
> This way makes no performance regression, maybe faster? since we don't need to
> scan markers.

Ok, good idea.

I've been exploring this and I've modified also how "contenteditable" works
(somehow reverting https://codereview.chromium.org/2650183002
and following the new approach).

If I manually test this new patch in Chrome it works, however the tests
are failing now. I'm sure I'm missing something else,
but I'd like to get your feedback again.
Note that I'm calling SpellChecker::cancelCheck(), but I'm not sure
if that's expected.

Thank you very much for your reviews!

[Xiaocheng, 2017-03-03 18:24:06]

Thanks for the update.

Unfortunately, both options that yosin gave might involve quite some low-level
details...

1. For the "idle marker removal" option, the idea is that SpellChecker can
request an asynchronous task that removes markers via
Document::requestIdleCallback. The same idea is used in idle time spell checker
(see IdleSpellCheckCallback.cpp).

However, idle tasks are not well supported in layout tests...
- By default, idle tasks without timeout are never run, while idle tasks with
timeouts work just like timeouts
- We can use testRunner.runIdleTasks() to force running idle tasks. However,
this function is not very well designed, and is quite tricky to use. I was using
it to test idle time spell checker, which created a lot of pain. I finally
decided not to use it. In fact, currently no layout test depends on it.

If we proceed with this option, we'll have to spend a lot of time
fixing/adapting existing test tools, and/or introducing new testing interfaces.
Not sure if it worths to invest so much time on a P3 issue.

2. For the "do not paint" option, the implemtation seems simple, but we need to
enhance Internals so that it can recognize such unpaint markers.

https://codereview.chromium.org/2669213002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/2669213002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLElement.cpp:434:
document().frame()->spellChecker().cancelCheck();
SpellChecker::cancelCheck is not the right function to call; it simply cancels
the pending spell check request, if any. For example, if we:

1. Paste something into an editable, which triggers a spell check request
2. Before the request is resolved, set another editable as non-editable

This patch makes step 2 call SpellChecker::cancelCheck, which simply cancels the
checking for the first editable

https://codereview.chromium.org/2669213002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/2669213002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:810: if
(!hasEditableStyle(*node) &&
This breaks the rendering of non-spelling markers, like textmatch.

https://codereview.chromium.org/2669213002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:837: break;
The check should be added here.

[Manuel Rego, 2017-03-24 10:14:46]

Thanks @Xiaocheng for the feedback.
Reading that, I'm not sure what's the best thing to do.

I understand "idle tasks" should eventually be supported on layout tests,
so one option could wait until that is done before fixing these issues.

As you say it's not a high priority issue, do you think we should explore
the other path (enhance Interals to recognize markers)?
Or is it better to forget about this issue at this point?

[Xiaocheng, 2017-03-24 17:42:50]

xiaochengh@chromium.org changed reviewers:
+ wangxianzhu@chromium.org

[Xiaocheng, 2017-03-24 17:44:45]

+wangxianzhu

Hi Xianzhu,

This patch tries to stop painting spelling and grammar markers in non-editable /
disabled / readonly / ... elements.

How hard is it if we want to enhance Internals to inspect whether a
DocumentMarker is actually painted or not?

Thanks!

[Xianzhu, 2017-03-24 18:28:56]

On 2017/03/24 17:44:45, Xiaocheng wrote:
> +wangxianzhu
> 
> Hi Xianzhu,
> 
> This patch tries to stop painting spelling and grammar markers in non-editable
/
> disabled / readonly / ... elements.
> 
> How hard is it if we want to enhance Internals to inspect whether a
> DocumentMarker is actually painted or not?
> 
> Thanks!

I would use a pixel test instead of checking through the internals API.

[Xianzhu, 2017-03-24 18:35:44]

On 2017/03/24 18:28:56, Xianzhu wrote:
> On 2017/03/24 17:44:45, Xiaocheng wrote:
> > +wangxianzhu
> > 
> > Hi Xianzhu,
> > 
> > This patch tries to stop painting spelling and grammar markers in
non-editable
> /
> > disabled / readonly / ... elements.
> > 
> > How hard is it if we want to enhance Internals to inspect whether a
> > DocumentMarker is actually painted or not?
> > 
> > Thanks!
> 
> I would use a pixel test instead of checking through the internals API.

Or you can create a unit test which checks the painted output in
PaintController. See PaintControllerPaintTest for example.

[Xiaocheng, 2017-03-24 19:50:18]

On 2017/03/24 at 18:35:44, wangxianzhu wrote:
> On 2017/03/24 18:28:56, Xianzhu wrote:
> > On 2017/03/24 17:44:45, Xiaocheng wrote:
> > > +wangxianzhu
> > > 
> > > Hi Xianzhu,
> > > 
> > > This patch tries to stop painting spelling and grammar markers in
non-editable
> > /
> > > disabled / readonly / ... elements.
> > > 
> > > How hard is it if we want to enhance Internals to inspect whether a
> > > DocumentMarker is actually painted or not?
> > > 
> > > Thanks!
> > 
> > I would use a pixel test instead of checking through the internals API.
> 
> Or you can create a unit test which checks the painted output in
PaintController. See PaintControllerPaintTest for example.

Pixel test is something we tried very hard to avoid...

I'll take a look at PaintControllerPaintTest. Thanks!