Closed Bug 1319560 Opened 8 years ago Closed 8 years ago

nsDisplayList::HitTest need a way to constrain test to only visible display items

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

Details

Attachments

(3 files)

Bug 1319560 Part 1: Add a mHitTestShouldStopAtFirstOpaque flag to nsDisplayListBuilder.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1319560 Part 2: Add a nsLayoutUtils::FrameForPointFlags value to only retrieve visible frames.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1319560 Part 3: Change nsDisplayList::HitTest to exit early when HitTestShouldStopAtFirstOpaque() is true.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details 
nsDisplayList::HitTest is the way we translate display lists into an ordered list of frames. This test is currently done without consideration for the actual visibility of the display items. That's fine for the current use as an event hit test, but not useful for analyzing the visual layering of the HitTest region. We need this feature to determine visibility of nsRange rects for find highlighting, in Bug 1302470.
Component: Graphics → Layout
Attachment #8813448 - Flags: review?(matt.woodrow)
Attachment #8813449 - Flags: review?(matt.woodrow)
Attachment #8813450 - Flags: review?(matt.woodrow)
Comment on attachment 8813448 [details]
Bug 1319560 Part 1: Add a mHitTestShouldStopAtFirstOpaque flag to nsDisplayListBuilder.

https://reviewboard.mozilla.org/r/94842/#review95072

Sorry, I should have mentioned this earlier before you changed all the files. This is probably much simpler to implement as a state bit on nsDisplayListBuilder (similar to WillComputePluginGeometry). That's what we do for similar things, and avoids needing to change the function signature everywhere.
Attachment #8813448 - Flags: review?(matt.woodrow)
Comment on attachment 8813449 [details]
Bug 1319560 Part 2: Add a nsLayoutUtils::FrameForPointFlags value to only retrieve visible frames.

https://reviewboard.mozilla.org/r/94844/#review95074

Looks good, assuming the last bit changes to builder.SetIgnoringInvisible(aFlags & IGNORE_INVISIBLE) or similar.
Attachment #8813449 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8813450 [details]
Bug 1319560 Part 3: Change nsDisplayList::HitTest to exit early when HitTestShouldStopAtFirstOpaque() is true.

https://reviewboard.mozilla.org/r/94846/#review95076

::: layout/painting/nsDisplayList.cpp:3327
(Diff revision 1)
>                                    nsTArray<nsIFrame*> *aOutFrames,
>                                    bool aOnlyIfVisible)
>  {
>    // Assume that any point in our background rect is a hit.
> -  if (mBackgroundRect.Intersects(aRect)) {
> +  if (mBackgroundRect.Intersects(aRect) &&
> +      (!aOnlyIfVisible || mThemeTransparency == nsITheme::eOpaque)) {

Can't a transparent theme still be partially opaque? We only want to skip them if they are entirely transparent.
Attachment #8813450 - Flags: review?(matt.woodrow) → review-
Comment on attachment 8813450 [details]
Bug 1319560 Part 3: Change nsDisplayList::HitTest to exit early when HitTestShouldStopAtFirstOpaque() is true.

https://reviewboard.mozilla.org/r/94846/#review95076

So, this patch implements only adding frames to the output list if they are (at least partially) opaque. I'm worried that there are so many different display item types, and we're only handling this new flag for some of them. It seems like many of the others need to implement this logic too, and it would be easy to miss some (and we have no test coverage of this).

Could we instead add all frames up to the first display item that is opaque (for the current pixel) and then early return hit test at that point. We could implement that as part of nsDisplayWrapList::HitTest, checking GetOpaqueRegion on each of the child items, and returning if the opaque area covers the hit test rect.

For your specific problem, this would still result in the <body> frame being part of the returned frame list, and still ahead of the frame for the text. This should be ok though, since the presence of the text frame (regardless of position in the returned list) should indicate that it is visible (to the best of our knowledge).

For the case where the text is covered by a partially transparent div, then the text frame should be still in the list, and we should highlight it. If the text frame is covered by a fully opaque div, then we will stop hit testing on the opaque div and the text frame won't be in the returned list at all.

The only remaining problem is that GetOpaqueRegion isn't entirely accurate (it's a low-bound approximation, and some items just return the empty rect), so we might end up thinking some text is visible when it isn't really. I'm not sure how to fix this other than just improving the GetOpaqueRegion implementations as necessary.
(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
> Could we instead add all frames up to the first display item that is opaque
> (for the current pixel) and then early return hit test at that point. We
> could implement that as part of nsDisplayWrapList::HitTest, checking
> GetOpaqueRegion on each of the child items, and returning if the opaque area
> covers the hit test rect.

I'm unsure how to proceed. I'm not sure how to make HitTest do a conditional early return without changing the method signature to add an additional parameter (which is the objection in comment 4).

If we don't make HitTest conditional, and instead make the display list builder only contain display items starting from the topmost opaque item and all partially transparent items above that, then the call to GetOpaqueRegion would seem to be needed in... nsIFrame::BuildDisplayListForStackingContext? If not there, where?
Flags: needinfo?(matt.woodrow)
(In reply to Brad Werth [:bradwerth] from comment #8)
 
> I'm unsure how to proceed. I'm not sure how to make HitTest do a conditional
> early return without changing the method signature to add an additional
> parameter (which is the objection in comment 4).

Something like:

if (aBuilder.OnlyHitTestVisibleItems() && item->GetOpaqueRegion().Contains(aRect)) {
  return;
}

It was less of an objection, and more just convention of how we pass around state in display list handling.
Flags: needinfo?(matt.woodrow)
Brad, are you able to progress with the info Matt provided in comment 9?
Flags: needinfo?(bwerth)
I've uploaded a new patch that implements the approach in comment 9. I think I've done it the way Matt was suggesting. It works for the testcase I've built for bug 1302470. Matt will re-review Part 3 of the patch to see if I got it right.
Flags: needinfo?(bwerth)
Comment on attachment 8813448 [details]
Bug 1319560 Part 1: Add a mHitTestShouldStopAtFirstOpaque flag to nsDisplayListBuilder.

https://reviewboard.mozilla.org/r/94842/#review96612
Attachment #8813448 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8813450 [details]
Bug 1319560 Part 3: Change nsDisplayList::HitTest to exit early when HitTestShouldStopAtFirstOpaque() is true.

https://reviewboard.mozilla.org/r/94846/#review96614
Attachment #8813450 - Flags: review?(matt.woodrow) → review+
Keywords: checkin-needed
Summary: nsDisplayList::HitTest and nsDisplayItem::HitTest need parameter to constrain test to only visible display items → nsDisplayList::HitTest need a way to constrain test to only visible display items
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c396937ada0f
Part 1: Add a mHitTestShouldStopAtFirstOpaque flag to nsDisplayListBuilder. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/707be7b4ca59
Part 2: Add a nsLayoutUtils::FrameForPointFlags value to only retrieve visible frames. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/d643dc8256bf
Part 3: Change nsDisplayList::HitTest to exit early when HitTestShouldStopAtFirstOpaque() is true. r=mattwoodrow
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: