Closed Bug 1302551 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(3 files, 1 obsolete file)

Attached patch platform changes (obsolete) — Splinter Review
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.
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.
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!
:-)

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).
Attachment #8790962 - Attachment is obsolete: true
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));
Assignee: nobody → mstange
Status: NEW → ASSIGNED
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 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
Attachment #8791460 - Flags: review?(bugs) → review+
(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 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.
Attachment #8791461 - Flags: review?(matt.woodrow) → review+
Pushed by kgupta@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/11f62de653a8
https://hg.mozilla.org/mozilla-central/rev/eb2ab5bb6930
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.