Bug 115734 – Changing the selection pseudo style does not trigger a selection repaint

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: Text

[Sergio Villar Senin]

Let's suppose that we have some CSS rules like:

h1.red::selection {
  background: red;
}
h1.green::selection {
  background: green;
}

and this HTML code in the body:

<h1 id="header" class="red">Hello World</h1>

a JavaScript code doing something this:

document.getElementById("header").className = green;

won't trigger a selection repaint (and it should) because we're only changing a pseudo style.

[Sergio Villar Senin]

Created attachment 200925 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 200925 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=200925&action=review

> Source/WebCore/rendering/RenderText.cpp:222
> +    if (diff == StyleDifferenceEqual && hasSelectedChildren() && (newStyle->hasPseudoStyle(SELECTION) || oldStyle->hasPseudoStyle(SELECTION)))

Is it expected that diff is StyleDifferenceEqual in this case?

[Sergio Villar Senin]

(In reply to comment #2)
> (From update of attachment 200925 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200925&action=review
> 
> > Source/WebCore/rendering/RenderText.cpp:222
> > +    if (diff == StyleDifferenceEqual && hasSelectedChildren() && (newStyle->hasPseudoStyle(SELECTION) || oldStyle->hasPseudoStyle(SELECTION)))
> 
> Is it expected that diff is StyleDifferenceEqual in this case?

At least is what I got from my testing, but I can double check if that's in the spec.

[Benjamin Poulain]

Comment on attachment 200925 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=200925&action=review

No idea about correctness, but I think there are enough issues with the test to have a second round:

> LayoutTests/ChangeLog:13
> +        * fast/backgrounds/selection-change-style-expected.html: Added.
> +        * fast/backgrounds/selection-change-style.html: Added.

Why is the test here and not in fast/repaint?

> LayoutTests/fast/backgrounds/selection-change-style-expected.html:4
> +<!--
> +This test checks that changing the selection style triggers a selection redraw.
> +-->

In the expectations?

> LayoutTests/fast/backgrounds/selection-change-style.html:4
> +<!--
> +This test checks that changing the selection style triggers a selection redraw.
> +-->

Since this test can be run manually, it would be better to have the text in the test + an explanation of the expected results for someone trying the test in their browser.

> LayoutTests/fast/backgrounds/selection-change-style.html:26
> +        setTimeout(function() {redDiv.className = "green";testRunner.notifyDone();}, 100);

Shouldn't this be using runRepaintTest?
Otherwise you will slow down testing quite a bit.

[Sergio Villar Senin]

(In reply to comment #4)
> (From update of attachment 200925 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200925&action=review
> 
> No idea about correctness, but I think there are enough issues with the test to have a second round:
> 
> > LayoutTests/ChangeLog:13
> > +        * fast/backgrounds/selection-change-style-expected.html: Added.
> > +        * fast/backgrounds/selection-change-style.html: Added.
> 
> Why is the test here and not in fast/repaint?

Just because there was already a similar test there, I don't mind moving it.

> > LayoutTests/fast/backgrounds/selection-change-style.html:4
> > +<!--
> > +This test checks that changing the selection style triggers a selection redraw.
> > +-->
> 
> Since this test can be run manually, it would be better to have the text in the test + an explanation of the expected results for someone trying the test in their browser.

OK, I'll move it into a <p>. Regarding the explanation of results I guess the <div> contents are pretty much what you ask for.
 
> > LayoutTests/fast/backgrounds/selection-change-style.html:26
> > +        setTimeout(function() {redDiv.className = "green";testRunner.notifyDone();}, 100);
> 
> Shouldn't this be using runRepaintTest?
> Otherwise you will slow down testing quite a bit.

I tried with runRepaintTest() but it looks like running it in onload it's "too early". The bug only appears after finishing all pending rendering operations because if for some reason the text is rendered again then the selection will be properly repainted as well. I used the timeout to try to isolate the selection class change as much as possible. There are other tests using a similar approach.

[Sergio Villar Senin]

Created attachment 202453 [details]
Patch

[Sergio Villar Senin]

So after checking how the differences in style are evaluated I can conclude that indeed StyleDifferenceEqual is expected as the function that compares changes between style changes do not take into account the selection. I don't think we should change that because it'll trigger some style recomputing that is not really needed.

And regarding using runRepaintTest is not really possible because that forces a style recalc, something that we want to avoid in the test (we want to check that just changing the style of the selection triggers a repaint of the selection).

[Simon Fraser (smfr)]

Comment on attachment 202453 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=202453&action=review

> Source/WebCore/rendering/RenderText.cpp:222
> +    if (diff == StyleDifferenceEqual && hasSelectedChildren() && (newStyle->hasPseudoStyle(SELECTION) || oldStyle->hasPseudoStyle(SELECTION)))
> +        view()->repaintSelection();

What if there was another style change that happened at the same time as the ::selection style change?

[Sergio Villar Senin]

(In reply to comment #8)
> (From update of attachment 202453 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202453&action=review
> 
> > Source/WebCore/rendering/RenderText.cpp:222
> > +    if (diff == StyleDifferenceEqual && hasSelectedChildren() && (newStyle->hasPseudoStyle(SELECTION) || oldStyle->hasPseudoStyle(SELECTION)))
> > +        view()->repaintSelection();
> 
> What if there was another style change that happened at the same time as the ::selection style change?

That's a good question, and it's the reason why I was so restrictive in the conditions. If any other style change happens then the element whose style has changed will trigger a style recalc. That style recalc will end up calling the paint() method of the ChromeClient which will force a repaint of the text and the selection. That's why this change is only needed in the special case that only the style of the selection is changed.

[Simon Fraser (smfr)]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 202453 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=202453&action=review
> > 
> > > Source/WebCore/rendering/RenderText.cpp:222
> > > +    if (diff == StyleDifferenceEqual && hasSelectedChildren() && (newStyle->hasPseudoStyle(SELECTION) || oldStyle->hasPseudoStyle(SELECTION)))
> > > +        view()->repaintSelection();
> > 
> > What if there was another style change that happened at the same time as the ::selection style change?
> 
> That's a good question, and it's the reason why I was so restrictive in the conditions. If any other style change happens then the element whose style has changed will trigger a style recalc. That style recalc will end up calling the paint() method of the ChromeClient which will force a repaint of the text and the selection. That's why this change is only needed in the special case that only the style of the selection is changed.

But repainting the selection is a different thing than repainting renderers. The selection can cover gaps between renderers, which will need to be repainted even if elements themselves repaint.

I'd expect to see problems with selection repaint when, say, 'color' also changes.

[Sergio Villar Senin]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (From update of attachment 202453 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=202453&action=review
> > > 
> > > > Source/WebCore/rendering/RenderText.cpp:222
> > > > +    if (diff == StyleDifferenceEqual && hasSelectedChildren() && (newStyle->hasPseudoStyle(SELECTION) || oldStyle->hasPseudoStyle(SELECTION)))
> > > > +        view()->repaintSelection();
> > > 
> > > What if there was another style change that happened at the same time as the ::selection style change?
> > 
> > That's a good question, and it's the reason why I was so restrictive in the conditions. If any other style change happens then the element whose style has changed will trigger a style recalc. That style recalc will end up calling the paint() method of the ChromeClient which will force a repaint of the text and the selection. That's why this change is only needed in the special case that only the style of the selection is changed.
> 
> But repainting the selection is a different thing than repainting renderers. The selection can cover gaps between renderers, which will need to be repainted even if elements themselves repaint.

I must be missing something because this potential problem that you mention should happen even without the patch, shouldn't it?

Just to ensure that we're on the same page, this patch only repaints selection when the selection style changes _and_ that's the only style change happening. For any other case the current behavior remains the same.

[Dave Hyatt]

Comment on attachment 202453 [details]
Patch

RenderText is the wrong place to put this. You might have nothing but a selected image and no selected text. You need to do this in RenderObject. Let's see a new patch that handles images as well as text.

[Sergio Villar Senin]

This is working as expected now