1302551 - Add an API to AnonymousContent to specify a background color "cut out" region

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

Attachment: platform changes

This will make the find bar highlighting dimming more efficient, see bug 1302522. I'm attaching a proof of concept patch that Mike can play around with.

Comment 1

[Markus Stange [:mstange]]

Attachment: FinderHighlighter.jsm changes to use the API

This is just to show how the API can be used. You'll still need to make the changes to keep around the mask node and only call setCutoutRectsForElement when the rects change instead of replacing the whole element.

Comment 2

[Mike de Boer [:mikedeboer]]

I approve of all of this!! It works marvelously. And the code is simpler than I'd imagined, but that's with all god things. We'll have to miss the rounded corners in the cutouts, but that's of little consequence.
Oh and it's blazing fast.

Don't worry about FinderHighlighter.jsm, I'll make it work there and do the code _removal_ as well.

Sweet, I'm psyched!

Comment 3

[Markus Stange [:mstange]]

:-)

If you want rounded corners again, you can additionally put elements on top of the dimmer. I'm envisioning something like:
<div style="position:absolute; left/top/width/height; overflow:hidden; maybe an additional box-shadow to get a stroke like in safari">
  <div style="height:100%; border-radius:4px; box-shadow: 0 0 0 6px hsla(0,0%,0%,0.35)"></div>
</div>
Does that make sense? The box-shadow of the inner element fills in a shape around the rounded rect, and the overflow:hidden around it clips it so that you don't get double dimming outside the cutout rect.

But if you do that, you should create one AnonymousContent per such element, so that you don't have to replace them all if one of them changes (or is added / removed).

Comment 4

[Markus Stange [:mstange]]

Attachment: Bug 1302551 - Add AnonymousContent::SetCutoutRectsForElement to allow a more efficient find bar highlighter implementation.

This part just sets a cutoutregion property on the element node and schedules a paint.
The next part will check for that property and create a display item for the element
in question.

Review commit: https://reviewboard.mozilla.org/r/78870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78870/

Comment 5

[Markus Stange [:mstange]]

Attachment: Bug 1302551 - Add nsDisplaySolidColorRegion and create it for elements that have the cutoutregion property set on them.

Review commit: https://reviewboard.mozilla.org/r/78872/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78872/

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8790966 [details] [diff] [review]
FinderHighlighter.jsm changes to use the API

>         for (let rect of rects) {
>-          maskContent.push(`<div xmlns="${kNSHTML}" style="${rectStyle}; top: ${rect.y}px;
>-            left: ${rect.x}px; height: ${rect.height}px; width: ${rect.width}px;"></div>`);
>+          allRects.push([rect.x, rect.y, rect.width, rect.height]);

I've made a small change to the API, so this will now need to be
            allRects.push(new DOMRect(rect.x, rect.y, rect.width, rect.height));

Comment 7

[Matt Woodrow (:mattwoodrow)]

Can you please explain a little more about what this is and why we need it?

It looks like it just overrides the background drawing to fill the specified region with the background color and blocks normal backgrounds from being drawn (and seems like a great patch to do that).

What does 'cutout' mean in this context, and how does it help the findbar?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8791460 [details]
Bug 1302551 - Add AnonymousContent::SetCutoutRectsForElement to allow a more efficient find bar highlighter implementation.

https://reviewboard.mozilla.org/r/78870/#review77502

Comment 9

[Markus Stange [:mstange]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
> Can you please explain a little more about what this is and why we need it?
> What does 'cutout' mean in this context, and how does it help the findbar?

Sure.

The new findbar highlighter wants to dim the whole page with a translucent black overlay (rgba(0,0,0,0.35)), except in those locations where there are occurrences of the search term. So the overlay needs to have holes in those places. The location of these holes is the "cutout region". (Would exclusion region be a better name?)

There are ways to achieve this effect with just CSS, but I couldn't come up with a way to do it that is both efficient to composite and has good invalidation behavior:
If you assemble the dimmed region using separate elements, you either need to find a very good way of recycling these elements, or you will invalidate large parts of the page as soon as one of the hole moves or resizes, because that completely changes the shape of your dimmed region and its decomposition into rectangles.
If you use an SVG mask, painting becomes very expensive, and we don't have good invalidation shortcuts for changes within the mask either.
If you find a way to create nsDisplayClearBackground items for the holes, then you need an intermediate surface during compositing.
You can't use blend modes for the effect because blend modes don't support operator source / operator clear and because the AnonymousContent overlay is in a separate stacking context from the page so you can't blend with the page itself.

The look that's currently on Nightly is wrong (by accident), and we want to fix it. What we have on Nightly at the moment causes translucent white to be on top of the search term occurrences, instead of total transparency.

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8791460 [details]
Bug 1302551 - Add AnonymousContent::SetCutoutRectsForElement to allow a more efficient find bar highlighter implementation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78870/diff/1-2/

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8791461 [details]
Bug 1302551 - Add nsDisplaySolidColorRegion and create it for elements that have the cutoutregion property set on them.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78872/diff/1-2/

Comment 12

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8791461 [details]
Bug 1302551 - Add nsDisplaySolidColorRegion and create it for elements that have the cutoutregion property set on them.

https://reviewboard.mozilla.org/r/78872/#review77708

Sounds good!

I actually missed the region 'Sub' operation that converts from the 'cutout' region to the one we want to fill which was the main source of confusion.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11f62de653a8
Add AnonymousContent::SetCutoutRectsForElement to allow a more efficient find bar highlighter implementation. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2ab5bb6930
Add nsDisplaySolidColorRegion and create it for elements that have the cutoutregion property set on them. r=mattwoodrow

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/11f62de653a8
https://hg.mozilla.org/mozilla-central/rev/eb2ab5bb6930