Bug 32285 – REGRESSION: With 2 highlighted lines of text in gmail/hotmail selecting Bold selects other 2 edit buttons automatically

• Component: HTML Editing
• Status: RESOLVED FIXED

[Enrica Casucci]

Created attachment 44483 [details]
Repro case

* STEPS TO REPRODUCE
1. login to gmail, create new mail msg
2. type any two lines of text (one [enter] two)
3. Select all  (Ctrl-A)
4. CLick Bold edit button

* RESULTS
All three buttons (Bold, Italic and Underline) get selected, text is modified according to 1 selection made by user.

[Enrica Casucci]

Created attachment 44489 [details]
Patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44489 [details] without any errors.

[Darin Adler]

Comment on attachment 44489 [details]
Patch

Enrica,

Did you look at <http://trac.webkit.org/changeset/46920>, the changeset that introduced this feature? Could you explain why the change wasn't needed to fix the bug that was being fixed back then? That changeset says something about "platforms other than Mac". Will this change of yours break some regression test on non-Mac platforms?

[Enrica Casucci]

(In reply to comment #3)
> (From update of attachment 44489 [details])
> Enrica,
> 
> Did you look at <http://trac.webkit.org/changeset/46920>, the changeset that
> introduced this feature? Could you explain why the change wasn't needed to fix
> the bug that was being fixed back then? That changeset says something about
> "platforms other than Mac". Will this change of yours break some regression
> test on non-Mac platforms?

I did look at the changeset and I honestly do not understand why it was done that way.
The original code was ignoring a number of style attributes for non-text nodes during the comparison.
Can I run the tests on Windows to verify that I haven't broken anything?

[Darin Adler]

I suggest sending mail to webkit-dev to see if anyone can remember. I don't think Justin or Ryosuke are going to be reachable.

> The original code was ignoring a number of style attributes for non-text nodes
> during the comparison.

Yes, I saw that. There must have been some reason to do so.

> Can I run the tests on Windows to verify that I haven't broken anything?

My guess is that if tests pass on both Mac and Windows (the patch says "platforms other than Mac", so those two platform should be enough) that means either you didn't break anything, or the tests checked in with the original change were insufficient.

[Ryosuke Niwa]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 44489 [details] [details])
> > Enrica,
> > 
> > Did you look at <http://trac.webkit.org/changeset/46920>, the changeset that
> > introduced this feature? Could you explain why the change wasn't needed to fix
> > the bug that was being fixed back then? That changeset says something about
> > "platforms other than Mac". Will this change of yours break some regression
> > test on non-Mac platforms?
> 
> I did look at the changeset and I honestly do not understand why it was done
> that way.
> The original code was ignoring a number of style attributes for non-text nodes
> during the comparison.
> Can I run the tests on Windows to verify that I haven't broken anything?

In platforms other than Mac, the entire selected text needs to have the specified style as supposed to just the beginning of selection in Mac.  And when the selection contains non-text-node such as div element that does not contain any text, WebKit used to detect that the style isn't present since elements without text nodes may lack the particular style.  The changeset 46920 was introduced to avoid this problem by ignoring non-text-nodes since those do not contribute to the style.

[Ryosuke Niwa]

To see this behavior, modify executeToggleStyle locally to use the default behavior:
http://trac.webkit.org/browser/trunk/WebCore/editing/EditorCommand.cpp#L157

To illustrate this behavioral change in Mac and other platforms, consider:
<b>hello</b>world

When this text is selected, query state for bold returns true for Mac but returns false for other platforms.

[Enrica Casucci]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 44489 [details] [details] [details])
> > > Enrica,
> > > 
> > > Did you look at <http://trac.webkit.org/changeset/46920>, the changeset that
> > > introduced this feature? Could you explain why the change wasn't needed to fix
> > > the bug that was being fixed back then? That changeset says something about
> > > "platforms other than Mac". Will this change of yours break some regression
> > > test on non-Mac platforms?
> > 
> > I did look at the changeset and I honestly do not understand why it was done
> > that way.
> > The original code was ignoring a number of style attributes for non-text nodes
> > during the comparison.
> > Can I run the tests on Windows to verify that I haven't broken anything?
> 
> In platforms other than Mac, the entire selected text needs to have the
> specified style as supposed to just the beginning of selection in Mac.  And
> when the selection contains non-text-node such as div element that does not
> contain any text, WebKit used to detect that the style isn't present since
> elements without text nodes may lack the particular style.  The changeset 46920
> was introduced to avoid this problem by ignoring non-text-nodes since those do
> not contribute to the style.
Thanks for the explanation, now I understand and I've verified that the fix breaks the test you added on platforms other than Mac. The original code still has a problem when you have something like
<b>hello</b><div><b>world</b></div> and you call queryCommandState("italic").
When looking at the first node the answer is false, but as you move to the div node the answer becomes true.
Now that I fully understand the goal of your patch, I'll post a new fix that takes into that into account.

[Ryosuke Niwa]

(In reply to comment #8)
> > In platforms other than Mac, the entire selected text needs to have the
> > specified style as supposed to just the beginning of selection in Mac.  And
> > when the selection contains non-text-node such as div element that does not
> > contain any text, WebKit used to detect that the style isn't present since
> > elements without text nodes may lack the particular style.  The changeset 46920
> > was introduced to avoid this problem by ignoring non-text-nodes since those do
> > not contribute to the style.
> Thanks for the explanation, now I understand and I've verified that the fix
> breaks the test you added on platforms other than Mac. The original code still
> has a problem when you have something like
> <b>hello</b><div><b>world</b></div> and you call queryCommandState("italic").
> When looking at the first node the answer is false, but as you move to the div
> node the answer becomes true.
> Now that I fully understand the goal of your patch, I'll post a new fix that
> takes into that into account.

Sorry about the regression, I guess I wasn't considering the case where there was no text-node. It seems like we need an extra condition that at least one text node has the given style.

[Enrica Casucci]

Created attachment 44640 [details]
Patch3

This patch fixes the issue for all the platforms.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44640 [details] without any errors.

[Enrica Casucci]

Committed revision 51965.