1302470 - Find in page should only find visible text

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect, P1)

Description

[Olli Pettay [:smaug]]

Attachment: screenshot

Comment 1

[Mike de Boer [:mikedeboer]]

I know what's going on here. nsFind doesn't do an extra-neat hit-test to see of the range is _truly_ visible.
So we need to add more sophisticated checking here.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: use a hit-test method to determine if the rect of a range is visible on the page or not to the eye

Hi Markus! This is what I could come up with for now. I takes care of other modal highlighting bugs - because typeAheadFind doesn't detect when a match is outside the viewport properly - but doesn't fix the bug on this page.
What I _need_ to know is whether the ranges' rects are obscured by another region that is rendered on top of them. I think the right term in layout-land is 'opaque'...

How would I go about getting that information? Do you have tips and can you tell me if I'm on the right track with this patch? Thanks!!

Comment 5

[Markus Stange [:mstange]]

(In reply to Mike de Boer [:mikedeboer] from comment #2)
> but doesn't fix the bug on this page.
> What I _need_ to know is whether the ranges' rects are obscured by another
> region that is rendered on top of them.

The problem on this page is not that something is *on top* of the text, it's that the text is clipped away by overflow:hidden. There's an element with class="shortcuts__categories" which has max-height: 0px; overflow: hidden; set on it, to hide the menu that appears when you click the arrow next to "Pikalinkit".

I think you need to iterate over all ancestor elements. For each ancestor element with overflow:hidden, test whether the bounding client rects of the ancestor and the target element intersect, and if they do not, then skip the target element.
I wouldn't bother doing actual occlusion testing. It's hard to get right and there are cases where an element is not visible at the moment but will be brought into view by scrolling, and the user should still be able to find those elements.

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to Markus Stange [:mstange] from comment #5)
> The problem on this page is not that something is *on top* of the text, it's
> that the text is clipped away by overflow:hidden. There's an element with
> class="shortcuts__categories" which has max-height: 0px; overflow: hidden;
> set on it, to hide the menu that appears when you click the arrow next to
> "Pikalinkit".

Alright, that's right in this case. But how do I get to know when the ranges' rects are obscured by another region that is rendered on top of them? Can I do that with a hit-test too?

> I think you need to iterate over all ancestor elements. For each ancestor
> element with overflow:hidden, test whether the bounding client rects of the
> ancestor and the target element intersect, and if they do not, then skip the
> target element.

I'm quite handy at searching for examples myself, but if you were to have one for me at the ready... ;-)

> I wouldn't bother doing actual occlusion testing. It's hard to get right and
> there are cases where an element is not visible at the moment but will be
> brought into view by scrolling, and the user should still be able to find
> those elements.

The thing is that for TypeAheadFind it's actually interesting to know that an element is not visible at that specific moment. If it were nsFind, where the iterator runs on, this would be the wrong approach. Thus, TBH, occlusion testing sounds like a good candidate to me. I'm a bit scared of the 'hard to get right' part... :( I mean, this patch already posed a bit of a challenge for poor old me.
I do feel layout/ is a cool part of the codebase to work with and experiment-friendly, to get a hang of available APIs and how things are tied together.

Comment 8

[Mike de Boer [:mikedeboer]]

Another example that this function should catch and motivates adding occlusion testing: https://bugzilla.mozilla.org/attachment.cgi?id=8793443

Comment 9

[Markus Stange [:mstange]]

(In reply to Mike de Boer [:mikedeboer] from comment #6)
> (In reply to Markus Stange [:mstange] from comment #5)
> > The problem on this page is not that something is *on top* of the text, it's
> > that the text is clipped away by overflow:hidden. There's an element with
> > class="shortcuts__categories" which has max-height: 0px; overflow: hidden;
> > set on it, to hide the menu that appears when you click the arrow next to
> > "Pikalinkit".
> 
> Alright, that's right in this case. But how do I get to know when the
> ranges' rects are obscured by another region that is rendered on top of
> them? Can I do that with a hit-test too?

Well, you can try... I don't have high hopes for this to work reliably.

> > I think you need to iterate over all ancestor elements. For each ancestor
> > element with overflow:hidden, test whether the bounding client rects of the
> > ancestor and the target element intersect, and if they do not, then skip the
> > target element.
> 
> I'm quite handy at searching for examples myself, but if you were to have
> one for me at the ready... ;-)

What I described here was the algorithm for finding out whether an element is clipped away by overflow:hidden, which would fix the case on yle.fi. I don't have other examples.

> > I wouldn't bother doing actual occlusion testing. It's hard to get right and
> > there are cases where an element is not visible at the moment but will be
> > brought into view by scrolling, and the user should still be able to find
> > those elements.
> 
> The thing is that for TypeAheadFind it's actually interesting to know that
> an element is not visible at that specific moment. If it were nsFind, where
> the iterator runs on, this would be the wrong approach.

Can you explain the differences between these two and why they need separate approaches?

> Thus, TBH, occlusion
> testing sounds like a good candidate to me. I'm a bit scared of the 'hard to
> get right' part...

Me too. Which of these cases does Safari handle? Can we restrict ourselves to handling the same ones?
For example, what if you have an element with a large border radius on top of the highlighted term, and the highlighted text is partly overlapped by the rounded edge? How much of the element is highlighted?

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Markus Stange [:mstange] from comment #9)
> Can you explain the differences between these two and why they need separate
> approaches?

Sure! nsFind iterates the document structure and finds all the occurrences that are physically present in a page. The fact that nsFind::IsVisibleNode() is in there is already a bug, because it's the responsibility of nsFind _consumers_ to inspect the ranges it gets for things like visibility, is it inside a link and all these other meta-operations.
nsTypeAheadFind is one these consumers, which calls nsFind::Find() consecutively until a favorable range is found (like being visible, among other criteria). Do you see now why I said that the presence of nsFind::IsVisibleNode() is a bug?
Another consumer is FinderIterator.jsm, which is used by Finder.jsm to count the amount of matches and FinderHighlighter.jsm to get all the rects to cut out. They would like to use nsTypeAheadFind::IsRangeVisible() when it's time to specify the cutouts, to determine the range is still visible on the page in its current state.

> Me too. Which of these cases does Safari handle? Can we restrict ourselves
> to handling the same ones?
> For example, what if you have an element with a large border radius on top
> of the highlighted term, and the highlighted text is partly overlapped by
> the rounded edge? How much of the element is highlighted?

Why would we restrict ourselves to what Safari does? I mean, Safari probably uses another system API (CoreText/ CoreGraphics or somesort) to do all this... it looks to be pretty exact wrt occlusion, but has its own bugs with tracking dynamic containers like position: fixed and iframes.
I was surprised to see that, TBH!

What would be the next steps here? Do you have an idea what I could implement or can you help out a bit - again? :-S

Comment 11

[Markus Stange [:mstange]]

(In reply to Mike de Boer [:mikedeboer] from comment #10)
> (In reply to Markus Stange [:mstange] from comment #9)
> > Can you explain the differences between these two and why they need separate
> > approaches?
> 
> Sure! nsFind iterates the document structure and finds all the occurrences
> that are physically present in a page. The fact that nsFind::IsVisibleNode()
> is in there is already a bug, because it's the responsibility of nsFind
> _consumers_ to inspect the ranges it gets for things like visibility, is it
> inside a link and all these other meta-operations.
> nsTypeAheadFind is one these consumers, which calls nsFind::Find()
> consecutively until a favorable range is found (like being visible, among
> other criteria). Do you see now why I said that the presence of
> nsFind::IsVisibleNode() is a bug?
> Another consumer is FinderIterator.jsm, which is used by Finder.jsm to count
> the amount of matches and FinderHighlighter.jsm to get all the rects to cut
> out. They would like to use nsTypeAheadFind::IsRangeVisible() when it's time
> to specify the cutouts, to determine the range is still visible on the page
> in its current state.

Ok, thanks.

> > Me too. Which of these cases does Safari handle? Can we restrict ourselves
> > to handling the same ones?
> > For example, what if you have an element with a large border radius on top
> > of the highlighted term, and the highlighted text is partly overlapped by
> > the rounded edge? How much of the element is highlighted?
> 
> Why would we restrict ourselves to what Safari does? I mean, Safari probably
> uses another system API (CoreText/ CoreGraphics or somesort) to do all
> this... it looks to be pretty exact wrt occlusion, but has its own bugs with
> tracking dynamic containers like position: fixed and iframes.
> I was surprised to see that, TBH!

Well, it's just code - it doesn't really matter where the code lives, be it a system library or not. The are trying to solve a very similar problem, and they have had to make very similar decisions and trade-offs between code complexity and correctness. There is a very long tail of small edge cases here that we don't handle properly, and the Safari developers will already have found out which of these cases are important to get right and which are not.

> What would be the next steps here? Do you have an idea what I could
> implement or can you help out a bit - again? :-S

The first step would be to come up with testcases and define the intended behavior on them. For example, when something is half clipped or half covered, do you want to exclude the match, or have the highlight only highlight the part that is visible? If you want both depending on some other condition (like the fraction of what's visible), what exactly are those conditions? If you have a transparent overlay on top of a match, do you still want to keep that match? (A transparent overlay blocks events, so nsLayoutUtils::GetFramesForArea will not include the match frame.) What if the overlay is not transparent, but has opacity:0.5?

Comment 12

[Mike de Boer [:mikedeboer]]

(In reply to Markus Stange [:mstange] from comment #11)
> Well, it's just code - it doesn't really matter where the code lives, be it
> a system library or not. The are trying to solve a very similar problem, and
> they have had to make very similar decisions and trade-offs between code
> complexity and correctness. There is a very long tail of small edge cases
> here that we don't handle properly, and the Safari developers will already
> have found out which of these cases are important to get right and which are
> not.

This matches my observations. I also saw that their implementation misses quite a number of cases, but so will we - most probably.

> The first step would be to come up with testcases and define the intended
> behavior on them. For example, when something is half clipped or half
> covered, do you want to exclude the match, or have the highlight only
> highlight the part that is visible? If you want both depending on some other
> condition (like the fraction of what's visible), what exactly are those
> conditions? If you have a transparent overlay on top of a match, do you
> still want to keep that match? (A transparent overlay blocks events, so
> nsLayoutUtils::GetFramesForArea will not include the match frame.) What if
> the overlay is not transparent, but has opacity:0.5?

Guiding principle with these conditions: 'perfect' is the enemy of 'good (enough)'.

1. Have two modes, each answering a different question;
1.1. Is it partly visible? Here, partly means a) the top-left corner of the first rect or b) the bottom right corner of the last rect. The amount of rects is usually just one, but we need to consider complex cases too.
1.2. Is it entirely visible? Here, entirely means BOTH the top-left corner of the first rect AND the bottom right corner of the last rect are visible.
1.3. If having two modes will take too much time to implement, I prefer to match _more_ instead of less, meaning having at least 1.1 implemented is most desirable.

If we're not going to compare points (t-l and b-r), but already have code that can determine the amount of overlap a ranges' rects have:
1.1.ad. Then, partly means more than half of the surface area of the rects combined must be visible.
1.2.ad. Then, entirely means all of the surface area of the rects combined must be visible.

2. If the following conditions for counting a range as visible make the implementation more complex, they should NOT be part of the implementation:
2.1. border-radius.
2.2. opacity. If it's doable within a reasonable amount of time, then the condition would be: any overlay on top of a ranges' rect with opacity < 1 should be counted as 'visible'.

Comment 13

[Mike de Boer [:mikedeboer]]

O, I forgot: What do you think about the behavior I described in comment 12?

Comment 15

[Markus Stange [:mstange]]

After thinking about this more, I think the problem here is quite similar to the problem of plugin visibility, and we might be able to re-use some of that infrastructure.

Timothy, what do you think about this:
 - Similar to the way the old findbar did "highlight all", we somehow cause nsDisplayItems for highlight ranges to show up in the display list. But they shouldn't render anything.
 - ComputeVisibility sets the actual visible rect on each of these nsDisplayItems
 - Then we somehow expose this stored rect to nsTypeAheadFind::IsRangeVisible (not sure how to identify the ranges with each other...)

Not sure whether the display list that we do this on should be the one that we use during painting, or one that's newly-generated for nsFind. Building a display list per find term occurrence is probably not fast enough.

I'm really looking for more ideas for how to move the information between layout and the place that needs it. Unfortunately I don't really know much about nsFind + consumers.

Comment 16

[Markus Stange [:mstange]]

Comment on attachment 8793280 [details] [diff] [review]
Patch v1: use a hit-test method to determine if the rect of a range is visible on the page or not to the eye

Are you still looking for feedback on this patch? I'm not sure what state this is in - does it work / improve things?

Comment 17

[Mike de Boer [:mikedeboer]]

(In reply to Markus Stange [:mstange] from comment #15)
> I'm really looking for more ideas for how to move the information between
> layout and the place that needs it. Unfortunately I don't really know much
> about nsFind + consumers.

I should be able to answer nsFind related questions. The rects we get from nsRange::GetClientRects(), which iterates over content leaves collecting TextFrames. But that you prolly already know :-)

(In reply to Markus Stange [:mstange] from comment #16)
> Are you still looking for feedback on this patch? I'm not sure what state
> this is in - does it work / improve things?

Well, it works in the sense that it filters out rects that are outside of the visible viewport, which is something we don't have at the moment. But it doesn't fix this bug, because the invisible ranges are inside the visible viewport here.
So basically I'd like to know if a given TextFrame is inside a stack of visibly rendered frames.

Comment 18

[Olli Pettay [:smaug]]

nsDOMWindowUtils::NodesFromRect doesn't help you here? Or is that too slow?
I mean for each rect you'd get all the nodes and then check whether the one you're interested in is visible or so.

Comment 19

[Markus Stange [:mstange]]

(In reply to Olli Pettay [:smaug] from comment #18)
> nsDOMWindowUtils::NodesFromRect doesn't help you here?

Hmm, maybe it does. You'd still need to walk the whole list to find the node you're interested in, and then check the styles of all the nodes on top. It might work, but it may also be slow.

Comment 20

[Mike de Boer [:mikedeboer]]

Hi Jet, this bug is blocking the new findbar highlighting feature to ride the trains further than just Nightly.
The thing that needs to be implemented is spec'ed in comment 12 and Olli, but especially Markus(!) have helped me a lot already here. The conclusion thusfar is that the implementation should be (re-)using several bits of already available functionality to tell the story of a range's visibility to the user reliably.
I'm requesting support similar to bug 1282752. Please, feel free to reach out on IRC if you need more info!

Thanks!

Comment 21

[Jet Villegas (inactive)]

Brad is having a look...

Comment 22

[Brad Werth [:bradwerth]]

On it. Please be patient while I get oriented...

Comment 23

[Brad Werth [:bradwerth]]

Attachment: testcase

Provides two failure examples when using typeahead-find:
a) Lower z-index "a" should not be selected
b) Overflow hidden "b" should not be selected

Comment 24

[Brad Werth [:bradwerth]]

Since the highlight rectangles are appearing on the OTHER find results, and not on the current one, the intervention in attachment 8793280 [details] [diff] [review] is not going to help. That patch affects the nsTypeAheadFind::IsRangeVisible method, which is only applied to the currently highlighted find result. The rectangles appear to be drawn by some JS method, which invokes nsFind::Find to get the ranges to highlight.

So... since nsFind::Find is doing the right thing (returning ranges regardless of visibility), then it seems like what is needed is a JS API that allows a script to query whether the returned ranges are visible. If that existed, then the same script that invokes Find could also invoke this new API on each of the returned ranges. Sound reasonable?

Comment 25

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #24)
> So... since nsFind::Find is doing the right thing (returning ranges
> regardless of visibility), then it seems like what is needed is a JS API
> that allows a script to query whether the returned ranges are visible. If
> that existed, then the same script that invokes Find could also invoke this
> new API on each of the returned ranges. Sound reasonable?

It occurs to me that this is the likely intended purpose of nsTypeAheadFind::IsRangeVisible. But that function is currently only being called for the current matching range. It needs to be called for all of the ranges returned by the invocation of nsFind::Find. Once that change is made, I can further tune the function to do all the visiblity tests laid out in comment 12. I don't know how to change that script -- can you upload a patch that accomplishes this?

Comment 26

[Mike de Boer [:mikedeboer]]

Attachment: Patch: only render visible ranges

Brad, you can see the favorable method definition in the patch I attached to this bug earlier as my first (unsuccessful) attempt:

```js
// Now the def is:
fastFind.isRangeVisible(range, docShell, mustBeInViewport);

// But instead, I'd imagine it to become:
fastFind.isRangeVisible(range, flushLayout);
// 1. docShell can be queried from the range, if necessary
// 2. 'must be in viewport' is always true.
```

So the one thing I think I did get right in my earlier patch is the method definition :P


This patch filters out invisible ranges at the moment of (re)drawing the rectangles in the mask overlay. This is intentional, because it may become visible later when the DOM or CSS changes on the page.

Please let me know if you need more info!

Comment 27

[Brad Werth [:bradwerth]]

Attachment: Bug 1302470 Part 1: Remove trailing whitespace.

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

Comment 28

[Brad Werth [:bradwerth]]

Attachment: Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

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

Comment 29

[Brad Werth [:bradwerth]]

Attachment: Bug 1302470 Part 3: Call the new isRangeVisible function to determine whether or not to draw a highlight rect.

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

Comment 30

[Brad Werth [:bradwerth]]

Attachment: Bug 1302470 Part 4: Augment the IsRangeVisible function to test for opaque overdraw.

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

Comment 31

[Markus Stange [:mstange]]

Comment on attachment 8809124 [details]
Bug 1302470 Part 1: Remove trailing whitespace.

https://reviewboard.mozilla.org/r/91768/#review91678

Comment 32

[Markus Stange [:mstange]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

https://reviewboard.mozilla.org/r/91770/#review91686

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp:1223
(Diff revision 1)
> +    RefPtr<mozilla::dom::DOMRect> rect = rects->Item(i);
> +    nsRect r(nsPresContext::CSSPixelsToAppUnits((float)rect->X()),
> +             nsPresContext::CSSPixelsToAppUnits((float)rect->Y()),
> +             nsPresContext::CSSPixelsToAppUnits((float)rect->Width()),
> +             nsPresContext::CSSPixelsToAppUnits((float)rect->Height()));
> +    nsLayoutUtils::GetFramesForArea(rootFrame, r, frames,

Please document in nsLayoutUtils.h above the GetFramesForArea method that the found frames are appended to aOutFrames, and the same with nsDisplayList::HitTest.
You're calling this function multiple times if the range has multiple rects, so you wouldn't want to overwrite the frames found for the previous rect.

Comment 33

[Markus Stange [:mstange]]

Comment on attachment 8809127 [details]
Bug 1302470 Part 4: Augment the IsRangeVisible function to test for opaque overdraw.

https://reviewboard.mozilla.org/r/91774/#review91688

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp:1236
(Diff revision 1)
> +  // Visibility requires the primary frame appears in the list returned from
> +  // GetFramesForArea, and that overlaying frames are all not entirely opaque.
> +  // Since GetFramesForArea returns frames in front-to-back order, stop
> +  // evaluating once we reach the primary frame, so we don't let frames
> +  // underneath the content affect the test.
> +  bool foundPrimaryFrame = false;

I think it's worth moving the whole loop out into a helper function so that you don't need this variable. And then in this function you'll just have
if (!FrameIsCompletelyVisibleOnTop(frame, frames)) {
  return false;
}

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp:1237
(Diff revision 1)
> +  // GetFramesForArea, and that overlaying frames are all not entirely opaque.
> +  // Since GetFramesForArea returns frames in front-to-back order, stop
> +  // evaluating once we reach the primary frame, so we don't let frames
> +  // underneath the content affect the test.
> +  bool foundPrimaryFrame = false;
> +  for (nsIFrame** f = frames.begin(); f != frames.end(); f++) {

You can use a ranged for loop here:
for (nsIFrame* f : frames) {

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp:1254
(Diff revision 1)
> +
> +    if ((*f)->StyleEffects()->mOpacity < 1.0f) {
> +      continue;
> +    }
> +
> +    // Something is covering the frame, so it's not visible.

Well, it's not as simple as that, and I think this comment should mention a few of the caveats. For example, this will return false even if just a single pixel of frame is overlapped by any other frame. It will also return false if the frames on top are completely transparent (for example if they don't have any background).

Comment 34

[Mike de Boer [:mikedeboer]]

Comment on attachment 8809124 [details]
Bug 1302470 Part 1: Remove trailing whitespace.

https://reviewboard.mozilla.org/r/91768/#review91698

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp:1011
(Diff revision 1)
>  
>      *aResult = FIND_FOUND;
>      return NS_OK;
>    }
>  
>    bool atEnd = false;    

You missed one! :) (Prolly more than one, but hmmkay)

Comment 35

[Mike de Boer [:mikedeboer]]

Comment on attachment 8809126 [details]
Bug 1302470 Part 3: Call the new isRangeVisible function to determine whether or not to draw a highlight rect.

https://reviewboard.mozilla.org/r/91772/#review91702

::: toolkit/modules/FinderHighlighter.jsm:1138
(Diff revision 1)
>      if (paintContent || dict.modalHighlightAllMask) {
>        this._updateDynamicRangesRects(dict);
>  
>        let DOMRect = window.DOMRect;
>        for (let [range, rects] of dict.modalHighlightRectsMap) {
> +        if (!this.finder._fastFind.isRangeVisible(range, this.iterator._getDocShell(range), true)) 

If you like, you can leave out this whole patch and up to me to fix things up in one of the bugs that depends on this one.
Thanks!
I am curious to hear from you what your experience was using your new implementation and having this in there?

Comment 36

[Olli Pettay [:smaug]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

https://reviewboard.mozilla.org/r/91770/#review91716

::: toolkit/components/typeaheadfind/nsITypeAheadFind.idl:61
(Diff revision 1)
>     * matches are owned by the same selection controller, this doesn't
>     * necessarily happen automatically. */
>    void collapseSelection();
>  
>    /* Check if a range is visible */
> -  boolean isRangeVisible(in nsIDOMRange aRange, in boolean aMustBeInViewPort);
> +  boolean isRangeVisible(in nsIDOMRange aRange, in boolean aFlushLayout);

Did you check if addons use this method and whether this kind of change is ok to them?

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp:1206
(Diff revision 1)
>    aRange->GetStartContainer(getter_AddRefs(node));
>  
>    nsCOMPtr<nsIContent> content(do_QueryInterface(node));
>    if (!content)
>      return false;
>  

I would expect us to flush layout somewhere here, when aFlushLayout is true, so that content->GetPrimaryFrame() returns up-to-date value.

So take a *strong* reference to content->OwnerDoc()->GetShell() and call FlushPendingNotifications on it

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp:1216
(Diff revision 1)
> -  if (!frame->StyleVisibility()->IsVisible())
> +  // Having a primary frame doesn't mean that the range is visible inside the
> +  // viewport. Do a hit-test to determine that quickly and properly.
> +  AutoTArray<nsIFrame*,8> frames;
> +  nsIFrame *rootFrame = aPresShell->GetRootFrame();
> +  RefPtr<nsRange> range = static_cast<nsRange*>(aRange);
> +  RefPtr<mozilla::dom::DOMRectList> rects = range->GetClientRects(true, aFlushLayout);

Then you could pass false as second param to GetClientRects

Comment 37

[Brad Werth [:bradwerth]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

https://reviewboard.mozilla.org/r/91770/#review91716

> Did you check if addons use this method and whether this kind of change is ok to them?

I have not. Since Mike de Boer provided this portion of the patch, and since I don't have contacts within our addons group, I defer to Mike on how to test whether this is a problematic change.

Comment 38

[Olli Pettay [:smaug]]

You can check addons code using dxr. Switch to addons tree.

Comment 39

[Olli Pettay [:smaug]]

https://dxr.mozilla.org/addons/source/

Comment 40

[Brad Werth [:bradwerth]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

https://reviewboard.mozilla.org/r/91770/#review91686

> Please document in nsLayoutUtils.h above the GetFramesForArea method that the found frames are appended to aOutFrames, and the same with nsDisplayList::HitTest.
> You're calling this function multiple times if the range has multiple rects, so you wouldn't want to overwrite the frames found for the previous rect.

https://hg.mozilla.org/mozilla-central/file/tip/layout/base/nsLayoutUtils.h#l824 has language for this already, and so does https://hg.mozilla.org/mozilla-central/file/tip/layout/base/nsDisplayList.h#l1396 .

Comment 41

[Markus Stange [:mstange]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

https://reviewboard.mozilla.org/r/91770/#review91686

> https://hg.mozilla.org/mozilla-central/file/tip/layout/base/nsLayoutUtils.h#l824 has language for this already, and so does https://hg.mozilla.org/mozilla-central/file/tip/layout/base/nsDisplayList.h#l1396 .

The current language doesn't specify what happens to the existing contents of the array. In the layout utils case, it currently sounds like they get overwritten, but they don't. I'd like this to be made more explicit - the frames that are found get *appended* to the array.

Comment 42

[Brad Werth [:bradwerth]]

Comment on attachment 8809126 [details]
Bug 1302470 Part 3: Call the new isRangeVisible function to determine whether or not to draw a highlight rect.

https://reviewboard.mozilla.org/r/91772/#review91702

> If you like, you can leave out this whole patch and up to me to fix things up in one of the bugs that depends on this one.
> Thanks!
> I am curious to hear from you what your experience was using your new implementation and having this in there?

It works well in a non-automated testcase I'm using (attached to the bug). I can't comment on performance. Seems like this call could be an expensive one. We need a testcase that tests functionality and possibly another that tests performance.

Comment 43

[Brad Werth [:bradwerth]]

Comment on attachment 8809124 [details]
Bug 1302470 Part 1: Remove trailing whitespace.

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

Comment 44

[Brad Werth [:bradwerth]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

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

Comment 45

[Brad Werth [:bradwerth]]

Comment on attachment 8809126 [details]
Bug 1302470 Part 3: Call the new isRangeVisible function to determine whether or not to draw a highlight rect.

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

Comment 46

[Brad Werth [:bradwerth]]

Comment on attachment 8809127 [details]
Bug 1302470 Part 4: Augment the IsRangeVisible function to test for opaque overdraw.

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

Comment 47

[Brad Werth [:bradwerth]]

Comment on attachment 8809126 [details]
Bug 1302470 Part 3: Call the new isRangeVisible function to determine whether or not to draw a highlight rect.

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

Comment 48

[Brad Werth [:bradwerth]]

Comment on attachment 8809127 [details]
Bug 1302470 Part 4: Augment the IsRangeVisible function to test for opaque overdraw.

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

Comment 49

[Brad Werth [:bradwerth]]

Comment on attachment 8809126 [details]
Bug 1302470 Part 3: Call the new isRangeVisible function to determine whether or not to draw a highlight rect.

https://reviewboard.mozilla.org/r/91772/#review91702

> It works well in a non-automated testcase I'm using (attached to the bug). I can't comment on performance. Seems like this call could be an expensive one. We need a testcase that tests functionality and possibly another that tests performance.

I'm going to leave this part of the patch in, since the feature is untested without it.

Comment 50

[Brad Werth [:bradwerth]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

https://reviewboard.mozilla.org/r/91770/#review91716

> I have not. Since Mike de Boer provided this portion of the patch, and since I don't have contacts within our addons group, I defer to Mike on how to test whether this is a problematic change.

I can't tell what impact this change has from looking at https://dxr.mozilla.org/addons/search?q=israngevisible&redirect=true. I'm uncertain how to interpret that I see there.

Comment 51

[Mike de Boer [:mikedeboer]]

Comment on attachment 8809126 [details]
Bug 1302470 Part 3: Call the new isRangeVisible function to determine whether or not to draw a highlight rect.

https://reviewboard.mozilla.org/r/91772/#review92012

::: toolkit/modules/FinderHighlighter.jsm:1138
(Diff revision 3)
>      if (paintContent || dict.modalHighlightAllMask) {
>        this._updateDynamicRangesRects(dict);
>  
>        let DOMRect = window.DOMRect;
>        for (let [range, rects] of dict.modalHighlightRectsMap) {
> +        if (!this.finder._fastFind.isRangeVisible(range, this.iterator._getDocShell(range), true))

OK, then can you make this `!this.finder._fastFind.isRangeVisible(range, false)`?

Comment 52

[Brad Werth [:bradwerth]]

Comment on attachment 8809126 [details]
Bug 1302470 Part 3: Call the new isRangeVisible function to determine whether or not to draw a highlight rect.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91772/diff/3-4/

Comment 53

[Brad Werth [:bradwerth]]

Comment on attachment 8809127 [details]
Bug 1302470 Part 4: Augment the IsRangeVisible function to test for opaque overdraw.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91774/diff/3-4/

Comment 54

[Brad Werth [:bradwerth]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

https://reviewboard.mozilla.org/r/91770/#review91716

> I can't tell what impact this change has from looking at https://dxr.mozilla.org/addons/search?q=israngevisible&redirect=true. I'm uncertain how to interpret that I see there.

Seems like this has stalled out. I want to be clear that I do not know how to resolve this issue to smaug's satisfaction. As a consequence, I won't be marking this issue as either fixed nor dropped.

Comment 55

[Mike de Boer [:mikedeboer]]

(In reply to Brad Werth [:bradwerth] from comment #54)
> > I can't tell what impact this change has from looking at https://dxr.mozilla.org/addons/search?q=israngevisible&redirect=true. I'm uncertain how to interpret that I see there.
> 
> Seems like this has stalled out. I want to be clear that I do not know how
> to resolve this issue to smaug's satisfaction. As a consequence, I won't be
> marking this issue as either fixed nor dropped.

So sorry for not replying here earlier, Brad. There is no impact for addons, when I checked out the DXR results _before_ it went on the fritz. All the identifiers you see there are custom implementations in JS with the same name, not (ab)using the nsITypeAheadFind method.
(This did not surprise me.)

Comment 56

[Carsten Book [:Tomcat]]

markus: can you look at part 3 , seems this still need a finished review so we can push this via the autolander. Thanks!

Comment 57

[Markus Stange [:mstange]]

I think Brad needs to re-push with r?mikedeboer in the commit message so that MozReview can pick up Mike's review. I haven't reviewed part 3 and I'm not the right person to do it.

Comment 58

[Brad Werth [:bradwerth]]

Comment on attachment 8809124 [details]
Bug 1302470 Part 1: Remove trailing whitespace.

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

Comment 59

[Brad Werth [:bradwerth]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

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

Comment 60

[Brad Werth [:bradwerth]]

Comment on attachment 8809126 [details]
Bug 1302470 Part 3: Call the new isRangeVisible function to determine whether or not to draw a highlight rect.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91772/diff/4-5/

Comment 61

[Brad Werth [:bradwerth]]

Comment on attachment 8809127 [details]
Bug 1302470 Part 4: Augment the IsRangeVisible function to test for opaque overdraw.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91774/diff/4-5/

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a71cde237471
Part 1: Remove trailing whitespace. r=mstange
https://hg.mozilla.org/integration/autoland/rev/8243da10a171
Part 2: Use a hit-test method to determine if the rect of a range is visible on the page or not to the eye, for use in find-in-page. r=mstange,smaug
https://hg.mozilla.org/integration/autoland/rev/3d34b2ac22a3
Part 3: Call the new isRangeVisible function to determine whether or not to draw a highlight rect. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/811c1368e51b
Part 4: Augment the IsRangeVisible function to test for opaque overdraw. r=mstange

Comment 63

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

Backed out for failing mochitests browser_Finder.js and test_browserElement_inproc_Find.html and various test failures in mochitest c3 on Linux x64 asan+debug (at least):

https://hg.mozilla.org/integration/autoland/rev/c010db9d4302df3bb5002b9da43aeaea30e907ad
https://hg.mozilla.org/integration/autoland/rev/cb78abd0c2a7e65034ae32418f9922e7fc036fa2
https://hg.mozilla.org/integration/autoland/rev/e8dd85621a7daeceba293bb7b6c1ccb76bce8e3e
https://hg.mozilla.org/integration/autoland/rev/5c0e1535fc906ba614f7092a2a735c33670331a0

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=811c1368e51ba421f27ec765c58b4e2151d26f30

bc3: TEST-UNEXPECTED-FAIL | toolkit/modules/tests/browser/browser_Finder.js | should find string - false == true - JS frame :: chrome://mochitests/content/browser/toolkit/modules/tests/browser/browser_Finder.js :: <TOP_LEVEL> :: line 33

c1:
TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_Find.html | test 7
TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_Find.html | test 8

c3:
TEST-UNEXPECTED-FAIL | leakcheck | default process: 4043997 bytes leaked (AbstractThread, AbstractWatcher, AnimationTimeline, AsyncLatencyLogger, AsyncStatement, ...)
TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_bug263683.xul | Correctly added a match to the selection type - 0 == 1
TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_bug263683.xul | Test timed out.
TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_findbar_entireword.xul | 'and' should've been found - didn't expect 1, but got it
...

Comment 64

[Brad Werth [:bradwerth]]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #63)
> Backed out for failing mochitests browser_Finder.js and
> test_browserElement_inproc_Find.html and various test failures in mochitest

It looks like the mochitests fail due to a calling pattern of the new function, which is beyond my ability to root cause and fix. I'm going to re-submit the patch with the activation disabled (as suggested in comment 35).

Comment 65

[Brad Werth [:bradwerth]]

Comment on attachment 8809124 [details]
Bug 1302470 Part 1: Remove trailing whitespace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91768/diff/3-4/

Comment 66

[Brad Werth [:bradwerth]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91770/diff/3-4/

Comment 67

[Brad Werth [:bradwerth]]

Comment on attachment 8809127 [details]
Bug 1302470 Part 4: Augment the IsRangeVisible function to test for opaque overdraw.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91774/diff/5-6/

Comment 68

[Ryan VanderMeulen [:RyanVM]]

(In reply to Brad Werth [:bradwerth] from comment #64)
> It looks like the mochitests fail due to a calling pattern of the new
> function, which is beyond my ability to root cause and fix. I'm going to
> re-submit the patch with the activation disabled (as suggested in comment
> 35).

Sounds like this might warrant a follow-up bug?

Comment 69

[Mike de Boer [:mikedeboer]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #68)
> Sounds like this might warrant a follow-up bug?

Or I'll set leave-open on this one and take over from there once Brad's patches have landed.

Comment 70

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e7c190a25bfa
Part 1: Remove trailing whitespace. r=mstange
https://hg.mozilla.org/integration/autoland/rev/cc04b61c9f23
Part 2: Use a hit-test method to determine if the rect of a range is visible on the page or not to the eye, for use in find-in-page. r=mstange,smaug
https://hg.mozilla.org/integration/autoland/rev/3ce8e7ccf045
Part 3: Augment the IsRangeVisible function to test for opaque overdraw. r=mstange

Comment 71

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

Backed out again for the same bc and c1 failure mentioned in comment 63:

https://hg.mozilla.org/integration/autoland/rev/36d96f69b36cf514ca10edbb67fef99321792a34
https://hg.mozilla.org/integration/autoland/rev/f0444d4065160afa13d7aa61c1bd73d4321354ae
https://hg.mozilla.org/integration/autoland/rev/2ddb3e67d33a11913dab00e372bad0a913ce0bba

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3ce8e7ccf045dafe326157cbd29af2eeebf5518e

Failure log bc: https://treeherder.mozilla.org/logviewer.html#?job_id=6911841&repo=autoland
11:07:44     INFO - TEST-START | toolkit/modules/tests/browser/browser_Finder.js
11:07:45     INFO - TEST-INFO | started process screentopng
11:07:46     INFO - TEST-INFO | screentopng: exit 0
11:07:46     INFO - Buffered messages logged at 11:07:44
11:07:46     INFO - Entering test bound 
11:07:46     INFO - Buffered messages logged at 11:07:45
11:07:46     INFO - Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "data:text/html;base64,PGJvZHk+PGlmcmFtZSBzcmNkb2M9ImNvbnRlbnQiLz48L2lmcmFtZT48YSBocmVmPSJodHRwOi8vdGVzdC5jb20iPnRlc3QgbGluazwvYT4=" line: 0}]
11:07:46     INFO - Buffered messages finished
11:07:46     INFO - TEST-UNEXPECTED-FAIL | toolkit/modules/tests/browser/browser_Finder.js | should find string - false == true - JS frame :: chrome://mochitests/content/browser/toolkit/modules/tests/browser/browser_Finder.js :: <TOP_LEVEL> :: line 33
11:07:46     INFO - Stack trace:
11:07:46     INFO -     chrome://mochitests/content/browser/toolkit/modules/tests/browser/browser_Finder.js:null:33
11:07:46     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:737:9
11:07:46     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:657:7
11:07:46     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:744:59

Failure log chrome test: https://treeherder.mozilla.org/logviewer.html#?job_id=6909549&repo=autoland
[task 2016-11-20T19:00:44.432077Z] 19:00:44     INFO - TEST-START | dom/browser-element/mochitest/test_browserElement_inproc_Find.html
[task 2016-11-20T19:00:45.648794Z] 19:00:45     INFO - TEST-INFO | started process screentopng
[task 2016-11-20T19:00:46.263491Z] 19:00:46     INFO - TEST-INFO | screentopng: exit 0
[task 2016-11-20T19:00:46.264546Z] 19:00:46     INFO - Buffered messages logged at 19:00:44
[task 2016-11-20T19:00:46.265835Z] 19:00:46     INFO - TEST-PASS | dom/browser-element/mochitest/test_browserElement_inproc_Find.html | test 0 
[task 2016-11-20T19:00:46.267510Z] 19:00:46     INFO - Buffered messages logged at 19:00:45
[task 2016-11-20T19:00:46.268859Z] 19:00:46     INFO - TEST-PASS | dom/browser-element/mochitest/test_browserElement_inproc_Find.html | test 1 
[task 2016-11-20T19:00:46.270358Z] 19:00:46     INFO - TEST-PASS | dom/browser-element/mochitest/test_browserElement_inproc_Find.html | test 2 
[task 2016-11-20T19:00:46.271544Z] 19:00:46     INFO - TEST-PASS | dom/browser-element/mochitest/test_browserElement_inproc_Find.html | test 3 
[task 2016-11-20T19:00:46.272768Z] 19:00:46     INFO - TEST-PASS | dom/browser-element/mochitest/test_browserElement_inproc_Find.html | test 4 
[task 2016-11-20T19:00:46.273805Z] 19:00:46     INFO - TEST-PASS | dom/browser-element/mochitest/test_browserElement_inproc_Find.html | test 5 
[task 2016-11-20T19:00:46.275106Z] 19:00:46     INFO - TEST-PASS | dom/browser-element/mochitest/test_browserElement_inproc_Find.html | test 6 
[task 2016-11-20T19:00:46.276461Z] 19:00:46     INFO - Buffered messages finished
[task 2016-11-20T19:00:46.277523Z] 19:00:46     INFO - TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_Find.html | test 7 
[task 2016-11-20T19:00:46.278483Z] 19:00:46     INFO - runTest/<@chrome://mochitests/content/chrome/dom/browser-element/mochitest/browserElement_Find.js:112:5
[task 2016-11-20T19:00:46.279478Z] 19:00:46     INFO - promise callback*runTest@chrome://mochitests/content/chrome/dom/browser-element/mochitest/browserElement_Find.js:31:3
[task 2016-11-20T19:00:46.280516Z] 19:00:46     INFO - browserElementTestHelpers.unlockTestReady@chrome://mochitests/content/chrome/dom/browser-element/mochitest/browserElementTestHelpers.js:47:7
[task 2016-11-20T19:00:46.281301Z] 19:00:46     INFO - Not taking screenshot here: see the one that was previously logged
[task 2016-11-20T19:00:46.282017Z] 19:00:46     INFO - TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_Find.html | test 8 
[task 2016-11-20T19:00:46.282640Z] 19:00:46     INFO - runTest/<@chrome://mochitests/content/chrome/dom/browser-element/mochitest/browserElement_Find.js:123:5
[task 2016-11-20T19:00:46.283266Z] 19:00:46     INFO - promise callback*runTest@chrome://mochitests/content/chrome/dom/browser-element/mochitest/browserElement_Find.js:31:3
[task 2016-11-20T19:00:46.283908Z] 19:00:46     INFO - browserElementTestHelpers.unlockTestReady@chrome://mochitests/content/chrome/dom/browser-element/mochitest/browserElementTestHelpers.js:47:7

Comment 72

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

The leak from comment 63 is also back: https://treeherder.mozilla.org/logviewer.html#?job_id=6912699&repo=autoland

[task 2016-11-20T19:16:13.566879Z] 19:16:13     INFO - TEST-INFO | leakcheck | default process: leaked 1 nsZipReaderCache
[task 2016-11-20T19:16:13.567097Z] 19:16:13     INFO - TEST-INFO | leakcheck | default process: leaked 145 xpc::CompartmentPrivate
[task 2016-11-20T19:16:13.567430Z] 19:16:13     INFO - TEST-INFO | leakcheck | default process: leaked 2 xpcJSWeakReference
[task 2016-11-20T19:16:13.568277Z] 19:16:13     INFO - TEST-INFO | leakcheck | default process: leaked 181 xptiInterfaceInfo
[task 2016-11-20T19:16:13.568873Z] 19:16:13     INFO - TEST-INFO | leakcheck | default process: leaked 1 xptiWorkingSet
[task 2016-11-20T19:16:13.569404Z] 19:16:13  WARNING - TEST-UNEXPECTED-FAIL | leakcheck | default process: 4113749 bytes leaked (AbstractThread, AbstractWatcher, AnimationTimeline, AsyncLatencyLogger, AsyncStatement, ...)
[task 2016-11-20T19:16:13.570528Z] 19:16:13     INFO - runtests.py | Running tests: end.

Comment 73

[Mike de Boer [:mikedeboer]]

I think I'll take care of the test updates and such things to make sure this'll be in a landable state.
In other words: I'll take it from here, Brad. Thanks so much!

Comment 74

[Mike de Boer [:mikedeboer]]

Brad, my initial testing shows that your work is doing its job a bit too well and causes the following issue:
Any range behind a semi-transparent background is considered invisible. In the case of modal highlighting, this is all the ranges: I show a semi-transparent background overlaying the page entirely and then cut-out the range rects, all using the AnonymousContent API (please see dom/base/AnonymousContent.h/cpp).
Is it possible to account for this, or perhaps even discard the effect the Anonymous Content layer has on the content below it?

Comment 75

[Mike de Boer [:mikedeboer]]

On second thought, I might be doing something wrong - but I'd like to check this case with you regardless.

Comment 76

[Brad Werth [:bradwerth]]

My code should treat any overlay with opacity < 1 as non-obscuring. Translation: transparency should be fine. I'll check with a testcase of my own. If you have a testcase you like, please feel free to attach it.

Comment 77

[Markus Stange [:mstange]]

The overlay might be using opacity:1 with background-color:rgba(0,0,0, something < 1).

Comment 78

[Brad Werth [:bradwerth]]

Confirmed there's a problem. I'll figure it out and get an updated Part 3 uploaded soon.

Comment 79

[Mike de Boer [:mikedeboer]]

Another problem I just found is that on about:home nsTypeAheadFind is _not_ finding anything past one character: it finds 'd', but not 'do' from 'Downloads'. Searching for 'b' (from 'Bookmarks') yields no results at all.
I confirmed that this does work without your patches applied.

Comment 80

[Brad Werth [:bradwerth]]

Attachment: testcase

Illustrates various conditions where text behind other elements should or should-not be included in find highlighting.

Comment 81

[Brad Werth [:bradwerth]]

(In reply to Markus Stange [:mstange] from comment #77)
> The overlay might be using opacity:1 with background-color:rgba(0,0,0,
> something < 1).

Bug 1319560 filed and it now has a patch that will be part of the solution. When it lands, I'll provide an updated patch that solves this issue. Comment 79 is a different issue that I'm still working through.

Comment 82

[Brad Werth [:bradwerth]]

Attachment: Bug 1302470 Part 3: Call the new isRangeVisible function to determine whether or not to draw a highlight rect.

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

Comment 83

[Brad Werth [:bradwerth]]

Attachment: Bug 1302470 Part 3: Fix the case where HTML buttons need to generate display item children when doing opaque hit tests.

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

Comment 84

[Brad Werth [:bradwerth]]

Comment on attachment 8809124 [details]
Bug 1302470 Part 1: Remove trailing whitespace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91768/diff/4-5/

Comment 85

[Brad Werth [:bradwerth]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91770/diff/4-5/

Comment 86

[Brad Werth [:bradwerth]]

Comment on attachment 8809127 [details]
Bug 1302470 Part 4: Augment the IsRangeVisible function to test for opaque overdraw.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91774/diff/6-7/

Comment 87

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #81)
> Bug 1319560 filed and it now has a patch that will be part of the solution.
> When it lands, I'll provide an updated patch that solves this issue. Comment
> 79 is a different issue that I'm still working through.

Bug 1319560 landed and I figured out a solution to the issue in comment 79. The proposed patch handles the testcase in attachment 8813347 [details] and the "about:home" testcase. More testcases are welcome.

Comment 88

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8816177 [details]
Bug 1302470 Part 3: Fix the case where HTML buttons need to generate display item children when doing opaque hit tests.

https://reviewboard.mozilla.org/r/96950/#review97748

Comment 89

[Mike de Boer [:mikedeboer]]

Brad, can you land the patches you haven't landed yet? The FinderHighlighter code changes need a bit more TLC from me, primarily fixing a few unit tests.
Thanks!

Comment 90

[Mike de Boer [:mikedeboer]]

I tested the latest patches as well and it works as expected in the cases I've quickly run by. \o/

Comment 91

[Brad Werth [:bradwerth]]

(In reply to Mike de Boer [:mikedeboer] from comment #89)
> Brad, can you land the patches you haven't landed yet? The FinderHighlighter
> code changes need a bit more TLC from me, primarily fixing a few unit tests.
> Thanks!

Mike, I'm glad it's working as intended. Because I don't have commit access to inbound, I'm hesitant to try to land just parts 1, 2, 4, and 5. The only approach I know would work would be resubmitting the patch with part 3 missing, and I don't want to make things any more confusing by doing that. If you are able to land them, it would probably be easier for you to land 1, 2, 4, and 5.

Comment 92

[Mike de Boer [:mikedeboer]]

OK, sure, I will!

If you need someone to vouch for you to get L3 access, please feel free to ask me.

Comment 93

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20a450dbe866

Comment 94

[Mike de Boer [:mikedeboer]]

Comment on attachment 8816176 [details]
Bug 1302470 Part 3: Call the new isRangeVisible function to determine whether or not to draw a highlight rect.

https://reviewboard.mozilla.org/r/96948/#review98170

Comment 95

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b92678afff8770634c1125fd48d63f6a0c3ba23
Bug 1302470 Part 1: Remove trailing whitespace. r=mstange

https://hg.mozilla.org/integration/mozilla-inbound/rev/b92b78271941789fc06c7b9ec7cd9c566b7fc20a
Bug 1302470 Part 2: Use a hit-test method to determine if the rect of a range is visible on the page or not to the eye, for use in find-in-page. r=mstange,smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d2569996ebc5b4625f9a221c78c00cb982a8735
Bug 1302470 Part 3: Call the new isRangeVisible function to determine whether or not to draw a highlight rect. r=mikedeboer

https://hg.mozilla.org/integration/mozilla-inbound/rev/113f7b13475cc8de15f8d253cee720e3a9df8ce1
Bug 1302470 Part 4: Augment the IsRangeVisible function to test for opaque overdraw. r=mstange

https://hg.mozilla.org/integration/mozilla-inbound/rev/005adbf78cd24afd81a24902ad8802d1b1d0c5aa
Bug 1302470 Part 5: Fix the case where HTML buttons need to generate display item children when doing opaque hit tests. r=mattwoodrow

Comment 96

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5b92678afff8
https://hg.mozilla.org/mozilla-central/rev/b92b78271941
https://hg.mozilla.org/mozilla-central/rev/3d2569996ebc
https://hg.mozilla.org/mozilla-central/rev/113f7b13475c
https://hg.mozilla.org/mozilla-central/rev/005adbf78cd2

Comment 97

[roxleitan]

Verified as fixed using the latest Nightly 53.0a1 (Build ID: 20161212030206) on Windows 10, Ubuntu 16.04 and Mac OS X 10.11

Comment 98

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed them all out in https://hg.mozilla.org/mozilla-central/rev/34a1ab064cb5b868fa75cb74d052e978eb34d6c1 for bug 1323200.

Comment 99

[Brad Werth [:bradwerth]]

Here's my read of the situation: we've got two distinct visibility checks that we're trying to handle in the same isRangeVisible function:

1) Is this range within the viewport?
2) Is this range rendered in such a way that the user can see it?

To answer question 1, we'll need that aIsRangeInViewport parameter or something similar. Or we could split this check into two functions since the algorithm is very different. That's solvable. But there's another problem. Since the display list is only created for visible content, the only way to answer question 2 is if the answer to question 1 is "yes".

In other words, our planned approach won't handle this case:

<div style="height: 5000px"></div>
<div style="display:hidden">text</div>

With the current plan, searching for "text", would return 1 result and scroll down to the bottom of the page, where nothing would be highlighted. Before I take another run at this code, I'd like to have a plan for handling this case also.

Comment 100

[Mike de Boer [:mikedeboer]]

It needs to basically support the things as discussed in bug 1323200, which caused the backout... 
In your example above, I think you meant 'visibility:hidden'? It should not find anything in that case (no results, no scrolling).

Comment 101

[Brad Werth [:bradwerth]]

(In reply to Mike de Boer [:mikedeboer] from comment #100)
> In your example above, I think you meant 'visibility:hidden'? It should not
> find anything in that case (no results, no scrolling).

Understood. We can mostly get there but we will still fall short because the display items aren't generated for out-of-viewport ranges. That's a key optimization that we need to retain. The only solution I can imagine is generating the whole page's display items when find is triggered, so we can do the full visibility test. That's pretty heavyweight and I'd propose that it's not necessary for the majority of our testcases.

I think the best solution to this is to give you a new method, IsRangeRendered that does the display list test, and to leave the current IsRangeVisible method unchanged. Then your algorithm would be to count and scroll to all ranges that pass IsRangeVisible (which does some basic checks that handle the visibility:hidden case and some other things) and then draw highlights on anything that passes IsRangeRangered. That would handle all the cases we've discussed except a case similar to the one in comment 99, where the found result is out of viewport and covered up by something else. In that case we'd still get a found result, and we'd scroll to a part of the page with no visible content (and no highlight drawn). We can trigger this in Nightly now on the yle.fi page by first scrolling to the bottom, then searching for "luonto", which is text that appears in the covered-up "Pikalinkit" menu at the top.

I'd like to proceed with this as a known failure case to avoid the rearchitecting of our display item generation.

Comment 102

[Brad Werth [:bradwerth]]

Comment on attachment 8809124 [details]
Bug 1302470 Part 1: Remove trailing whitespace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91768/diff/5-6/

Comment 103

[Brad Werth [:bradwerth]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91770/diff/5-6/

Comment 104

[Brad Werth [:bradwerth]]

Comment on attachment 8816177 [details]
Bug 1302470 Part 3: Fix the case where HTML buttons need to generate display item children when doing opaque hit tests.

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

Comment 105

[Mike de Boer [:mikedeboer]]

Thanks for the explanation, Brad. I understand it now and I'm looking forward to trying out the new `IsRangeRendered` method :-)

Comment 106

[Brad Werth [:bradwerth]]

I don't think there's anything I can do to move this forward until the plan in comment 101 is implemented. That needs to be done in JS code that Mike modified in attachment 8809126 [details] (since obsoloted). I really think Mike can implement this more assuredly than I can. If this needinfo sticks around on me for awhile, I'll give it a shot.

Comment 107

[Mike de Boer [:mikedeboer]]

(In reply to Brad Werth [:bradwerth] from comment #106)
> I don't think there's anything I can do to move this forward until the plan
> in comment 101 is implemented. That needs to be done in JS code that Mike
> modified in attachment 8809126 [details] (since obsoloted). I really think
> Mike can implement this more assuredly than I can. If this needinfo sticks
> around on me for awhile, I'll give it a shot.

But in comment 101 you mentioned to add an IsRangeRendered method that does the display list test?
I can add the IsRangeVisible calls there, but that's rather expensive in this hot code path.
Would this be different if the modal UI was drawn & rendered by platform entirely?
Can you see a way to allow ranges to be styled like you see on Nightly currently by platform?

Comment 108

[Brad Werth [:bradwerth]]

(In reply to Mike de Boer [:mikedeboer] from comment #107)
> But in comment 101 you mentioned to add an IsRangeRendered method that does
> the display list test?
> I can add the IsRangeVisible calls there, but that's rather expensive in
> this hot code path.

With this approach, IsRangeVisible doesn't change, so it's as fast as it ever was. The new IsRangeRendered method should also be fairly fast since it would only be triggered after the page has scrolled to the (likely visible) match, and so the display list is generated already and the traversal for each potential match within the viewport isn't too heavyweight. Try it out and see what you think.

> Would this be different if the modal UI was drawn & rendered by platform
> entirely?
> Can you see a way to allow ranges to be styled like you see on Nightly
> currently by platform?
That would seem to be a viable alternate approach, to style the existing element the way you are currently styling the overlaid element. I'm not sure where that code is, but if we decide this approach is the wrong one, then I'll look into it.

Comment 109

[Mike de Boer [:mikedeboer]]

I couldn't sleep last night, so I thought about this instead: I think we're dismissing our previous approach too soon, just because it showed regressions.
Now, what if we branch in ::IsRangeVisible() to use the hit-test method of checking visibility if and when we can cheaply tell whether the range is positioned inside the current viewport. If the range is out of view, (re-)use the current method(s).

How does that sound?

Comment 110

[Brad Werth [:bradwerth]]

(In reply to Mike de Boer [:mikedeboer] from comment #109)
> Now, what if we branch in ::IsRangeVisible() to use the hit-test method of
> checking visibility if and when we can cheaply tell whether the range is
> positioned inside the current viewport. If the range is out of view,
> (re-)use the current method(s).

I'll try something like that soon, as an additional step built on the current proposed patch (we'll keep the IsRangeRendered method).

Comment 111

[Brad Werth [:bradwerth]]

Attachment: Bug 1302470 Part 2: Branch IsRangeVisible to delegate to IsRangeRendered when range is in viewport.

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

Comment 112

[Brad Werth [:bradwerth]]

Comment on attachment 8809124 [details]
Bug 1302470 Part 1: Remove trailing whitespace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91768/diff/6-7/

Comment 113

[Brad Werth [:bradwerth]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91770/diff/6-7/

Comment 114

[Brad Werth [:bradwerth]]

Comment on attachment 8816177 [details]
Bug 1302470 Part 3: Fix the case where HTML buttons need to generate display item children when doing opaque hit tests.

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

Comment 115

[Brad Werth [:bradwerth]]

(In reply to Mike de Boer [:mikedeboer] from comment #109)
> Now, what if we branch in ::IsRangeVisible() to use the hit-test method of
> checking visibility if and when we can cheaply tell whether the range is
> positioned inside the current viewport. If the range is out of view,
> (re-)use the current method(s).

I've implemented this approach, but it seems there may be something else needed in the JS. IsRangeVisible is returning false but highlights are still being drawn on non-visible text. Let me know if you're getting an unexpected return value in your testing.

Comment 116

[Mike de Boer [:mikedeboer]]

Attachment: Patch: Finder frontend changes to tie things together

Brad, I applied your patches and this one atop it. It looks like it's working really well!!
(Including the cases that caused the previous iteration to be backed out)

Comment 117

[Mike de Boer [:mikedeboer]]

Markus, when do you think you'll be able to review this patch? If your queue is full, would there be, perhaps, someone you can defer this review to? Thanks!!

Comment 119

[Mike de Boer [:mikedeboer]]

Brad, I reached out to Markus two days ago - wanted to let you know what's up with the delay here:

[19:30:52] <mikedeboer> mstange: do you have time to (delegate) review brad's patch in bug 1302470?
[19:50:50] <mstange> mikedeboer: I don't know who to delegate it to, except maybe bz
[19:51:48] <mstange> mikedeboer: sorry that I'm taking so long  with that patch; I didn't do a very good job with it last time and I wanted to look at it more closely and also refresh my mind about the history of this code (and check what the previous patches did and how they failed)
[19:52:10] <mstange> mikedeboer: unfortunately this has meant that I did other simpler reviews first and kept bumping this one up my queue

Comment 120

[Mike de Boer [:mikedeboer]]

Markus, how's your queue doing these days? Any chance for this one to be the next item in it? You'll need to bite this (bitter) pill eventually! ;-)

Comment 121

[Markus Stange [:mstange]]

I've finally started looking at this patch. I'm really sorry for the long delay!

The patch itself seems fine. But the result isn't working as well as I hoped it would:
 - On http://searchfox.org/ , we still highlight words that are hidden by the fixed bar at the top.
 - On http://yle.fi/ , if you scroll quickly, you can see that we highlight rectangles outside the current viewport for words that aren't actually visible, and then as those areas enter the viewport we quickly correct ourselves and hide them.

Oh, I see that comment 99 was about this case. Nevermind the second observation then.

Do you know why the first case doesn't work?

Comment 122

[Brad Werth [:bradwerth]]

(In reply to Markus Stange [:mstange] from comment #121)
> I've finally started looking at this patch. I'm really sorry for the long
> delay!
> 
> The patch itself seems fine. But the result isn't working as well as I hoped
> it would:
>  - On http://searchfox.org/ , we still highlight words that are hidden by
> the fixed bar at the top.

Good catch. In this case, the display item is a nsDisplayFixedPosition for an nsBlockFrame with a HTMLFormElement as its content. In the hit test, this nsDisplayFixedPosition item is tested for opacity, which goes to https://dxr.mozilla.org/mozilla-central/source/layout/painting/nsDisplayList.cpp#5261. In this case, the mList is not opaque, so the function returns an empty region.

I'm not clear why that's correct logic. It may just be an optimization that works well enough for purposes of event bubbling (the primary use case of hit tests). Matt Woodrow will know. It seems that even if mList is not opaque, some of the elements in it could still be opaque. I'll attempt a patch that provides an else case that iterates the mList and does testing for the items in it.

Comment 123

[Brad Werth [:bradwerth]]

Attachment: Bug 1302470 Part 5: Connect up FinderHighlighter.jsm with the new isRangeVisible function.

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

Comment 124

[Brad Werth [:bradwerth]]

Comment on attachment 8809124 [details]
Bug 1302470 Part 1: Remove trailing whitespace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91768/diff/7-8/

Comment 125

[Brad Werth [:bradwerth]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91770/diff/7-8/

Comment 126

[Brad Werth [:bradwerth]]

Comment on attachment 8829007 [details]
Bug 1302470 Part 2: Branch IsRangeVisible to delegate to IsRangeRendered when range is in viewport.

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

Comment 127

[Brad Werth [:bradwerth]]

Comment on attachment 8816177 [details]
Bug 1302470 Part 3: Fix the case where HTML buttons need to generate display item children when doing opaque hit tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96950/diff/3-4/

Comment 128

[Brad Werth [:bradwerth]]

Attachment: Bug 1302470 Part 4: Change nsDisplayList::GetOpaqueRegion non-opaque lists to build up a region from its children.

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

Comment 129

[Brad Werth [:bradwerth]]

Comment on attachment 8851166 [details]
Bug 1302470 Part 5: Connect up FinderHighlighter.jsm with the new isRangeVisible function.

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

Comment 130

[Brad Werth [:bradwerth]]

(In reply to Markus Stange [:mstange] from comment #121)
>  - On http://searchfox.org/ , we still highlight words that are hidden by
> the fixed bar at the top.

The patch to nsDisplayList (attachment 8851170 [details]) sort of addresses this, but we still have these not-so-great user experiences. Steps to reproduce:

1) Navigate to http://searchfox.org .
2) Search for the word "welcome". Note that the only result is highlighted.
3) Scroll down so that the text disappears under the fixed position div at the top. The highlighted text will float over the fixed position div.

This would need to be fixed in the front-end to re-query the isRangeVisible function as the content scrolls. Continue on to see another problem:

4) Click on whitespace on the space to de-select the highlighted text.
5) Again search for "welcome". This time there are no matches found.
6) Scroll up so that the text reappears from under the fixed position div at the top. It will not be highlighted.

This problem may also be solved by having the front-end call the isRangeVisible function again as the content scrolls.

Comment 131

[Mike de Boer [:mikedeboer]]

(In reply to Brad Werth [:bradwerth] from comment #130)
> The patch to nsDisplayList (attachment 8851170 [details]) sort of addresses
> this, but we still have these not-so-great user experiences. Steps to
> reproduce:
> 
> 1) Navigate to http://searchfox.org .
> 2) Search for the word "welcome". Note that the only result is highlighted.
> 3) Scroll down so that the text disappears under the fixed position div at
> the top. The highlighted text will float over the fixed position div.
> 
> This would need to be fixed in the front-end to re-query the isRangeVisible
> function as the content scrolls. Continue on to see another problem:
> 
> 4) Click on whitespace on the space to de-select the highlighted text.
> 5) Again search for "welcome". This time there are no matches found.
> 6) Scroll up so that the text reappears from under the fixed position div at
> the top. It will not be highlighted.
> 
> This problem may also be solved by having the front-end call the
> isRangeVisible function again as the content scrolls.

That'd be something I'll need to take care of then once everything's finished up here.

Comment 132

[Mike de Boer [:mikedeboer]]

Comment on attachment 8851166 [details]
Bug 1302470 Part 5: Connect up FinderHighlighter.jsm with the new isRangeVisible function.

https://reviewboard.mozilla.org/r/123544/#review126292

Comment 133

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8851170 [details]
Bug 1302470 Part 4: Change nsDisplayList::GetOpaqueRegion non-opaque lists to build up a region from its children.

https://reviewboard.mozilla.org/r/123546/#review130098

This looks good, but I'm concerned that it adds extra work for the other callers of GetOpaqueRegion that they may not benefit from.

I think the easiest solution would be to pass a flag that requests a 'precise' opaque region, and only do the extra work there. Alternatively try collect data on how this more accurate opaque region benefits other parts of the pipeline, but I doubt that's worth it.

Comment 134

[Jet Villegas (inactive)]

Comment on attachment 8829007 [details]
Bug 1302470 Part 2: Branch IsRangeVisible to delegate to IsRangeRendered when range is in viewport.

Load-balancing this review to Masayuki

Comment 135

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8829007 [details]
Bug 1302470 Part 2: Branch IsRangeVisible to delegate to IsRangeRendered when range is in viewport.

https://reviewboard.mozilla.org/r/106218/#review131816

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp:1253
(Diff revision 2)
> +  bool isInViewport = false;
>    nsRectVisibility rectVisibility = nsRectVisibility_kAboveViewport;
>  
>    if (!aGetTopVisibleLeaf && !frame->GetRect().IsEmpty()) {
>      rectVisibility =
>        aPresShell->GetRectVisibility(frame,
>                                      nsRect(nsPoint(0,0), frame->GetSize()),
>                                      minDistance);
>  
>      if (rectVisibility != nsRectVisibility_kAboveViewport) {
> -      return true;
> +      isInViewport = true;
> +    }
> -    }
> +  }
> +
> +  if (isInViewport) {
> +    // This is an early exit case, where we return true iff the range
> +    // is actually rendered.
> +    return IsRangeRendered(aPresShell, aPresContext, aRange);
> +  }

I don't understand why you need to add isInViewport for here. You should just move this comment and return state into the |if| block.

Comment 136

[psn]

Attachment: highlight.png

Something like this was seen twice already.
It does not happen every time, and I cannot reproduce the bug at will.
Something like this: enter search string, then hide search bar.
On F3 key press search bar appears again, and it might "shift" the highlight as in the attached picture.

Comment 137

[Brad Werth [:bradwerth]]

Comment on attachment 8851170 [details]
Bug 1302470 Part 4: Change nsDisplayList::GetOpaqueRegion non-opaque lists to build up a region from its children.

https://reviewboard.mozilla.org/r/123546/#review130098

Sure, I'll use the new condition created in an earlier part of the patch "HitTestShouldStopAtFirstOpaque" to constrain the more expensive calculation to just this case.

Comment 138

[Brad Werth [:bradwerth]]

Comment on attachment 8809124 [details]
Bug 1302470 Part 1: Remove trailing whitespace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91768/diff/8-9/

Comment 139

[Brad Werth [:bradwerth]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91770/diff/8-9/

Comment 140

[Brad Werth [:bradwerth]]

Comment on attachment 8829007 [details]
Bug 1302470 Part 2: Branch IsRangeVisible to delegate to IsRangeRendered when range is in viewport.

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

Comment 141

[Brad Werth [:bradwerth]]

Comment on attachment 8816177 [details]
Bug 1302470 Part 3: Fix the case where HTML buttons need to generate display item children when doing opaque hit tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96950/diff/4-5/

Comment 142

[Brad Werth [:bradwerth]]

Comment on attachment 8851170 [details]
Bug 1302470 Part 4: Change nsDisplayList::GetOpaqueRegion non-opaque lists to build up a region from its children.

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

Comment 143

[Brad Werth [:bradwerth]]

Comment on attachment 8851166 [details]
Bug 1302470 Part 5: Connect up FinderHighlighter.jsm with the new isRangeVisible function.

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

Comment 144

[Brad Werth [:bradwerth]]

Attachment: Bug 1302470 Part 6: Properly check for a frame's visibility, do not abuse isRangeVisible() for that purpose.

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

Comment 145

[Brad Werth [:bradwerth]]

The mochitest toolkit/modules/tests/browser/browser_Finder.js fails when verifying that content in an iframe is highlighted correctly. I checked the new isRangeVisible function and confirmed that IT DOES report the iframe content as visible -- the failure to highlight is somewhere in the front-end code. For the moment, I have turned off that section of the test. Before we land this, Mike or someone else from the front-end team needs to file an appropriate follow-up bug, or supply an additional part of the patch to this bug in which case I will undo the changes in Part 7 that turn off the failing section of the test.

Comment 146

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8851170 [details]
Bug 1302470 Part 4: Change nsDisplayList::GetOpaqueRegion non-opaque lists to build up a region from its children.

https://reviewboard.mozilla.org/r/123546/#review135048

Comment 147

[Mike de Boer [:mikedeboer]]

Comment on attachment 8860170 [details]
Bug 1302470 Part 6: Properly check for a frame's visibility, do not abuse isRangeVisible() for that purpose.

https://reviewboard.mozilla.org/r/132194/#review135264

Fine by me, I'll take a look later.

Comment 148

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4212d9e1d0b
Part 1: Remove trailing whitespace. r=mstange
https://hg.mozilla.org/integration/autoland/rev/87ae43ab52ed
Part 2: Create a IsRangeRendered function to test range visibility in the display list. r=mstange,smaug
https://hg.mozilla.org/integration/autoland/rev/1d664865e8e2
Part 3: Branch IsRangeVisible to delegate to IsRangeRendered when range is in viewport. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/71de099b2eb7
Part 4: Fix the case where HTML buttons need to generate display item children when doing opaque hit tests. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/4fc16536ce41
Part 5: Change nsDisplayList::GetOpaqueRegion non-opaque lists to build up a region from its children. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/eb2930fc7c59
Part 6: Connect up FinderHighlighter.jsm with the new isRangeVisible function. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/855488862155
Part 7: Disable part of the browser_Finder.js test until Highlight All is fixed for iframe content. r=mikedeboer

Comment 149

[Carsten Book [:Tomcat]]

sorry had to back this out for eslint failures like https://treeherder.mozilla.org/logviewer.html#?job_id=93324759&repo=autoland

Comment 150

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d726a063ab02
Backed out changeset 855488862155 
https://hg.mozilla.org/integration/autoland/rev/40978edd88b7
Backed out changeset eb2930fc7c59 
https://hg.mozilla.org/integration/autoland/rev/28f8fc94cf1a
Backed out changeset 4fc16536ce41 
https://hg.mozilla.org/integration/autoland/rev/083edb20c643
Backed out changeset 71de099b2eb7 
https://hg.mozilla.org/integration/autoland/rev/85abcaa97076
Backed out changeset 1d664865e8e2 
https://hg.mozilla.org/integration/autoland/rev/71218b2d1f97
Backed out changeset 87ae43ab52ed 
https://hg.mozilla.org/integration/autoland/rev/a2561522f4cd
Backed out changeset a4212d9e1d0b for eslint failure

Comment 151

[Brad Werth [:bradwerth]]

Comment on attachment 8809124 [details]
Bug 1302470 Part 1: Remove trailing whitespace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91768/diff/9-10/

Comment 152

[Brad Werth [:bradwerth]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91770/diff/9-10/

Comment 153

[Brad Werth [:bradwerth]]

Comment on attachment 8829007 [details]
Bug 1302470 Part 2: Branch IsRangeVisible to delegate to IsRangeRendered when range is in viewport.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106218/diff/3-4/

Comment 154

[Brad Werth [:bradwerth]]

Comment on attachment 8816177 [details]
Bug 1302470 Part 3: Fix the case where HTML buttons need to generate display item children when doing opaque hit tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96950/diff/5-6/

Comment 155

[Brad Werth [:bradwerth]]

Comment on attachment 8851170 [details]
Bug 1302470 Part 4: Change nsDisplayList::GetOpaqueRegion non-opaque lists to build up a region from its children.

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

Comment 156

[Brad Werth [:bradwerth]]

Comment on attachment 8851166 [details]
Bug 1302470 Part 5: Connect up FinderHighlighter.jsm with the new isRangeVisible function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123544/diff/3-4/

Comment 157

[Brad Werth [:bradwerth]]

Comment on attachment 8860170 [details]
Bug 1302470 Part 6: Properly check for a frame's visibility, do not abuse isRangeVisible() for that purpose.

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

Comment 158

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f39e67694994
Part 1: Remove trailing whitespace. r=mstange
https://hg.mozilla.org/integration/autoland/rev/572662b36c77
Part 2: Create a IsRangeRendered function to test range visibility in the display list. r=mstange,smaug
https://hg.mozilla.org/integration/autoland/rev/a5a77d4a3cf3
Part 3: Branch IsRangeVisible to delegate to IsRangeRendered when range is in viewport. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/d0ecd711cbc4
Part 4: Fix the case where HTML buttons need to generate display item children when doing opaque hit tests. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/d12341c804a2
Part 5: Change nsDisplayList::GetOpaqueRegion non-opaque lists to build up a region from its children. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/726e98cd071e
Part 6: Connect up FinderHighlighter.jsm with the new isRangeVisible function. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/95aca3b5524e
Part 7: Disable part of the browser_Finder.js test until Highlight All is fixed for iframe content. r=mikedeboer

Comment 159

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

Backed out for failing mochitest test_bug263683.xul and more tests on opt-based builds on Linux and on debug on OSX, at least:

https://hg.mozilla.org/integration/autoland/rev/848bd3234045aec8495a6682a513e71f147eb47f
https://hg.mozilla.org/integration/autoland/rev/b22dc337ff39516ff1855ae2a41807dee0811b63
https://hg.mozilla.org/integration/autoland/rev/23656266698b0e59000c3b0fbe3b906a632ca3bd
https://hg.mozilla.org/integration/autoland/rev/f038fbe03508739e76f9b66bed6809878effec20
https://hg.mozilla.org/integration/autoland/rev/f9a14442e34b804c6f502482704781b498059d91
https://hg.mozilla.org/integration/autoland/rev/e0f5820cc75caf6f7a50db89b030caec01ec859e
https://hg.mozilla.org/integration/autoland/rev/2ebc1ba647232c8b757c4c343fd9e5d945f1c54d

Push with failures (the one which already got backed out, the second one only had a comment changed): https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=85548886215546769bdaa533223424151080a458&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=93335501&repo=autoland

[task 2017-04-21T15:42:48.431962Z] 15:42:48     INFO - TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Correctly added a match to the selection type - 1 == 1 
[task 2017-04-21T15:42:48.432035Z] 15:42:48     INFO - TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Added the correct match - "mozilla" == "mozilla" 
[task 2017-04-21T15:42:48.432947Z] 15:42:48     INFO - Buffered messages finished
[task 2017-04-21T15:42:48.433624Z] 15:42:48     INFO - TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_bug263683.xul | Correctly added a match to the selection type - 0 == 1 
[task 2017-04-21T15:42:48.434308Z] 15:42:48     INFO - resource://testing-common/content-task.js line 52 > eval:null:24
[task 2017-04-21T15:42:48.434935Z] 15:42:48     INFO - resource://gre/modules/Task.jsm:TaskImpl_run:319
[task 2017-04-21T15:42:48.435062Z] 15:42:48     INFO - resource://gre/modules/Task.jsm:TaskImpl:277
[task 2017-04-21T15:42:48.435636Z] 15:42:48     INFO - resource://gre/modules/Task.jsm:asyncFunction:252
[task 2017-04-21T15:42:48.435762Z] 15:42:48     INFO - resource://gre/modules/Task.jsm:Task_spawn:166
[task 2017-04-21T15:42:48.435883Z] 15:42:48     INFO - resource://testing-common/content-task.js:null:54
[task 2017-04-21T15:48:09.545811Z] 15:48:09     INFO - Not taking screenshot here: see the one that was previously logged

Comment 160

[Brad Werth [:bradwerth]]

Comment on attachment 8860170 [details]
Bug 1302470 Part 6: Properly check for a frame's visibility, do not abuse isRangeVisible() for that purpose.

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

Comment 161

[Brad Werth [:bradwerth]]

Attachment: Bug 1302470 Part 8: Turn off additional tests that fail on OS X, possibly due to unexpected successes in finding previously hidden text.

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

Comment 162

[Brad Werth [:bradwerth]]

Comment on attachment 8809124 [details]
Bug 1302470 Part 1: Remove trailing whitespace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91768/diff/10-11/

Comment 163

[Brad Werth [:bradwerth]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91770/diff/10-11/

Comment 164

[Brad Werth [:bradwerth]]

Comment on attachment 8829007 [details]
Bug 1302470 Part 2: Branch IsRangeVisible to delegate to IsRangeRendered when range is in viewport.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106218/diff/4-5/

Comment 165

[Brad Werth [:bradwerth]]

Comment on attachment 8816177 [details]
Bug 1302470 Part 3: Fix the case where HTML buttons need to generate display item children when doing opaque hit tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96950/diff/6-7/

Comment 166

[Brad Werth [:bradwerth]]

Comment on attachment 8851170 [details]
Bug 1302470 Part 4: Change nsDisplayList::GetOpaqueRegion non-opaque lists to build up a region from its children.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123546/diff/3-4/

Comment 167

[Brad Werth [:bradwerth]]

Comment on attachment 8851166 [details]
Bug 1302470 Part 5: Connect up FinderHighlighter.jsm with the new isRangeVisible function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123544/diff/4-5/

Comment 168

[Brad Werth [:bradwerth]]

Comment on attachment 8860170 [details]
Bug 1302470 Part 6: Properly check for a frame's visibility, do not abuse isRangeVisible() for that purpose.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132194/diff/3-4/

Comment 169

[Mike de Boer [:mikedeboer]]

Comment on attachment 8861486 [details]
Bug 1302470 Part 8: Turn off additional tests that fail on OS X, possibly due to unexpected successes in finding previously hidden text.

https://reviewboard.mozilla.org/r/133460/#review137624

I'd like to refrain from doing this, but find the cause of the failures instead. I mean, if we're finding previously hidden text, that should be happening on other platforms as well!
Another thing is that the iframe detection code can be found in FinderIterator.jsm; if you're uncomfortable diving into that code, would you like to debug things together over Vidyo or something?

The more direct method of working together we find, the better. I'm easily distracted nowadays (past month or so), so having a call, screen sharing and/ or chatting works best.

For now, I've got to r- this approach.

Comment 170

[Mike de Boer [:mikedeboer]]

(In the meantime I'm applying the patches locally to see if I can find what's going on as well.)

Comment 171

[Mike de Boer [:mikedeboer]]

Attachment: Fix instead of disable: properly check for a frame's visibility, do not abuse isRangeVisible() for that purpose

Brad, here's my fix for the FinderIterator, which fixes the problems tests were having. (Explanation of the code below.)
If your testing with this patch applied works for you as well, I'd advise to incorporate it into your patches and push everything to try before attempting to land it again. Just to be sure we're good ;-)

This patch does the following: using ::IsRangeVisible() was a hack to start with. This method is not designed to check if elements or windows are visible or not.
What the FinderIterator needs to know is the bare minimum - its task is to find as many ranges as possible, without doing unnecessary work. So a basic check for element dimensions may already be too much, because an iframe can be made visible again using JS at a later time. However, we'd rather take this optimization and count on the change detection mechanism as implemented in FinderHighlighter.jsm to be smart and re-request the search from FinderIterator later, when that happens.

So now I'm getting the rect of the frame element as cheaply as possible (using ::GetBoundsWithoutFlushing()) and - with a little help from Geometry.jsm's awesome Rect struct - determine that a it must not be an empty rect. Much more reliable and not taking ::IsRangeVisible() out of its context.

Comment 172

[Brad Werth [:bradwerth]]

Comment on attachment 8809124 [details]
Bug 1302470 Part 1: Remove trailing whitespace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91768/diff/11-12/

Comment 173

[Brad Werth [:bradwerth]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91770/diff/11-12/

Comment 174

[Brad Werth [:bradwerth]]

Comment on attachment 8829007 [details]
Bug 1302470 Part 2: Branch IsRangeVisible to delegate to IsRangeRendered when range is in viewport.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106218/diff/5-6/

Comment 175

[Brad Werth [:bradwerth]]

Comment on attachment 8816177 [details]
Bug 1302470 Part 3: Fix the case where HTML buttons need to generate display item children when doing opaque hit tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96950/diff/7-8/

Comment 176

[Brad Werth [:bradwerth]]

Comment on attachment 8851170 [details]
Bug 1302470 Part 4: Change nsDisplayList::GetOpaqueRegion non-opaque lists to build up a region from its children.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123546/diff/4-5/

Comment 177

[Brad Werth [:bradwerth]]

Comment on attachment 8851166 [details]
Bug 1302470 Part 5: Connect up FinderHighlighter.jsm with the new isRangeVisible function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123544/diff/5-6/

Comment 178

[Brad Werth [:bradwerth]]

Comment on attachment 8860170 [details]
Bug 1302470 Part 6: Properly check for a frame's visibility, do not abuse isRangeVisible() for that purpose.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132194/diff/4-5/

Comment 179

[Brad Werth [:bradwerth]]

Comment on attachment 8862930 [details] [diff] [review]
Fix instead of disable: properly check for a frame's visibility, do not abuse isRangeVisible() for that purpose

Review of attachment 8862930 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like a good simplification.

Comment 180

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0bb3e70e5f46
Part 1: Remove trailing whitespace. r=mstange
https://hg.mozilla.org/integration/autoland/rev/033fce5ecd39
Part 2: Create a IsRangeRendered function to test range visibility in the display list. r=mstange,smaug
https://hg.mozilla.org/integration/autoland/rev/cc6ff95187d0
Part 3: Branch IsRangeVisible to delegate to IsRangeRendered when range is in viewport. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/22793139a866
Part 4: Fix the case where HTML buttons need to generate display item children when doing opaque hit tests. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/3542d7bacbdc
Part 5: Change nsDisplayList::GetOpaqueRegion non-opaque lists to build up a region from its children. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/2394e63f50ff
Part 6: Connect up FinderHighlighter.jsm with the new isRangeVisible function. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/de9f42b512c8
Part 7: Properly check for a frame's visibility, do not abuse isRangeVisible() for that purpose. r=mikedeboer

Comment 181

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/dfbdf17fba00 for an armload of mochitest-chrome failures, https://treeherder.mozilla.org/logviewer.html#?job_id=95359407&repo=autoland

Comment 182

[Mike de Boer [:mikedeboer]]

This is just the weirdest thing... the ENTIRE suite is green, EXCEPT OSX 10.10 debug. I've tried to reproduce this, but couldn't reproduce it locally. It _must_ have been a fluke, but pushing the whole thing to try regardless.

Comment 183

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17625951ddcf251327676276c3a9282df9edb760

Comment 184

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c39b99fad60548c88dce5890343723c979dc11db

Comment 185

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Assuming 55 and 54 are affected as well. I'm not sure if this is a regression or not but let's call it one for now.

Comment 186

[Gerry Chang [:gchang]]

Too late for 54. Mark 54 won't fix.

Comment 187

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

Comment 188

[Brad Werth [:bradwerth]]

Mike, are you still interested in landing typeahead find? Let me know and I'll refresh the patch and fix any bitrot.

Comment 189

[Mike de Boer [:mikedeboer]]

Yes! If you're able to land this, I'll try very hard to get this running for Fx 57!

Comment 190

[Brad Werth [:bradwerth]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91770/diff/12-13/

Comment 191

[Brad Werth [:bradwerth]]

Comment on attachment 8829007 [details]
Bug 1302470 Part 2: Branch IsRangeVisible to delegate to IsRangeRendered when range is in viewport.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106218/diff/6-7/

Comment 192

[Brad Werth [:bradwerth]]

Comment on attachment 8816177 [details]
Bug 1302470 Part 3: Fix the case where HTML buttons need to generate display item children when doing opaque hit tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96950/diff/8-9/

Comment 193

[Brad Werth [:bradwerth]]

Comment on attachment 8851170 [details]
Bug 1302470 Part 4: Change nsDisplayList::GetOpaqueRegion non-opaque lists to build up a region from its children.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123546/diff/5-6/

Comment 194

[Brad Werth [:bradwerth]]

Comment on attachment 8851166 [details]
Bug 1302470 Part 5: Connect up FinderHighlighter.jsm with the new isRangeVisible function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123544/diff/6-7/

Comment 195

[Brad Werth [:bradwerth]]

Comment on attachment 8860170 [details]
Bug 1302470 Part 6: Properly check for a frame's visibility, do not abuse isRangeVisible() for that purpose.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132194/diff/5-6/

Comment 196

[Brad Werth [:bradwerth]]

Okay patch is refreshed. It doesn't look quite like I remember it -- I don't see the typeahead effect. It does, however, correctly skip over find results that aren't visible. I'd like you to look at it to see if it's doing what you expect. I'll attempt to build a test for the isRangeRendered function and add that to the patch.

Comment 197

[Mike de Boer [:mikedeboer]]

Thanks Brad! The effect has been disabled on Nightly for a couple of trains already, because it was taking too long to get it to a shippable state.
This is going to help lots and I'll make sure to pick things up where I left off after this puppy lands.

Comment 198

[Brad Werth [:bradwerth]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91770/diff/13-14/

Comment 199

[Brad Werth [:bradwerth]]

Comment on attachment 8829007 [details]
Bug 1302470 Part 2: Branch IsRangeVisible to delegate to IsRangeRendered when range is in viewport.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106218/diff/7-8/

Comment 200

[Brad Werth [:bradwerth]]

Comment on attachment 8816177 [details]
Bug 1302470 Part 3: Fix the case where HTML buttons need to generate display item children when doing opaque hit tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96950/diff/9-10/

Comment 201

[Brad Werth [:bradwerth]]

Comment on attachment 8851170 [details]
Bug 1302470 Part 4: Change nsDisplayList::GetOpaqueRegion non-opaque lists to build up a region from its children.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123546/diff/6-7/

Comment 202

[Brad Werth [:bradwerth]]

Comment on attachment 8851166 [details]
Bug 1302470 Part 5: Connect up FinderHighlighter.jsm with the new isRangeVisible function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123544/diff/7-8/

Comment 203

[Brad Werth [:bradwerth]]

Comment on attachment 8860170 [details]
Bug 1302470 Part 6: Properly check for a frame's visibility, do not abuse isRangeVisible() for that purpose.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132194/diff/6-7/

Comment 204

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/399011313b3c
Part 1: Create a IsRangeRendered function to test range visibility in the display list. r=mstange,smaug
https://hg.mozilla.org/integration/autoland/rev/6b948c533944
Part 2: Branch IsRangeVisible to delegate to IsRangeRendered when range is in viewport. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/160522290018
Part 3: Fix the case where HTML buttons need to generate display item children when doing opaque hit tests. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/a67bc2f1b624
Part 4: Change nsDisplayList::GetOpaqueRegion non-opaque lists to build up a region from its children. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/fdd40abac611
Part 5: Connect up FinderHighlighter.jsm with the new isRangeVisible function. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/3aab8b1494ef
Part 6: Properly check for a frame's visibility, do not abuse isRangeVisible() for that purpose. r=mikedeboer

Comment 205

[Phil Ringnalda (:philor)]

I certainly don't remember the boatload of failures from 4 months ago, but https://treeherder.mozilla.org/logviewer.html#?job_id=127579164&repo=autoland (Win8 opt) https://treeherder.mozilla.org/logviewer.html#?job_id=127581874&repo=autoland (Mac opt) https://treeherder.mozilla.org/logviewer.html#?job_id=127582518&repo=autoland (Mac debug) looks like the sort of thing I would have described as a boatload.

Backed out in https://hg.mozilla.org/integration/autoland/rev/628a46720153

Comment 206

[Mike de Boer [:mikedeboer]]

Brad, looking at the failing tests, it looks like your code isn't able to detect a ranges' visibility inside an input element or textarea. So perhaps that bit needs some more work?

Comment 207

[Brad Werth [:bradwerth]]

Attachment: input testcase

Comment 208

[Brad Werth [:bradwerth]]

(In reply to Mike de Boer [:mikedeboer] from comment #206)
> Brad, looking at the failing tests, it looks like your code isn't able to
> detect a ranges' visibility inside an input element or textarea. So perhaps
> that bit needs some more work?

Yes, there's something off with this implementation for finding text in input elements. From running the test locally, I've found some manual steps to reproduce with the testcase attachment 8906159 [details]:

1) Load the testcase.
2) Open the find toolbar and enter the text "inside".
3) Close the find toolbar.
4) Reload the testcase.
5) Open the find toolbar and press the Enter key to trigger find.

Expected results: the word "inside" will be selected in the input field. "1 of 1 match" will appear in the find toolbar area.
Actual results: nothing is selected. "Phrase not found" appears in the find toolbar area.

Anyway, being able to reproduce will speed me in finding a fix.

Comment 209

[Mike de Boer [:mikedeboer]]

Brad, will you be able to work on this in the near future? You'd make me so very happy when we can land this, you can't imagine! ;-)

Thanks for your efforts so far!

Comment 210

[Brad Werth [:bradwerth]]

bz, my testcase attachment 8906159 [details] and the steps to reproduce in comment 208 hits an NS_WARNING you put in nsTextFrame: "internal offsets may be out-of-sync". I'm hitting that warning in a call to nsTextFrame::GetPointFromOffset because the nsTextFrame has a state marked NS_FRAME_IS_DIRTY. This also causes the point to not be generated, and that makes the find fail when it shouldn't. Subsequent finds work correctly because the frame is no longer marked as dirty.

So, what is the nature of the exceptional case you are marking here? Do you have any advice for me on how I can get nsTextFrame::GetPointFromOffset to return a point on the just-loaded nsTextFrame in the testcase?

Comment 211

[Boris Zbarsky [:bzbarsky]]

I didn't add this warning, fwiw.  I just reviewed the patch that did, that being the fix for bug 307875.

> because the nsTextFrame has a state marked NS_FRAME_IS_DIRTY

Right, so if the frame is dirty then that means layout is not up-to-date and hence asking for a point from an offset will give you an answer that is at best stale and at worst completely nonsensical.

> Do you have any advice for me on how I can get nsTextFrame::GetPointFromOffset to
> return a point on the just-loaded nsTextFrame in the testcase?

Make sure that if a caller wants to know a point from the offset, and really needs to have a correct answer, then that code flushes out layout before asking for the point?

Comment 212

[Brad Werth [:bradwerth]]

Comment on attachment 8809125 [details]
Bug 1302470 Part 1: Create a IsRangeRendered function to test range visibility in the display list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91770/diff/14-15/

Comment 213

[Brad Werth [:bradwerth]]

Comment on attachment 8829007 [details]
Bug 1302470 Part 2: Branch IsRangeVisible to delegate to IsRangeRendered when range is in viewport.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106218/diff/8-9/

Comment 214

[Brad Werth [:bradwerth]]

Comment on attachment 8816177 [details]
Bug 1302470 Part 3: Fix the case where HTML buttons need to generate display item children when doing opaque hit tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96950/diff/10-11/

Comment 215

[Brad Werth [:bradwerth]]

Comment on attachment 8851170 [details]
Bug 1302470 Part 4: Change nsDisplayList::GetOpaqueRegion non-opaque lists to build up a region from its children.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123546/diff/7-8/

Comment 216

[Brad Werth [:bradwerth]]

Comment on attachment 8851166 [details]
Bug 1302470 Part 5: Connect up FinderHighlighter.jsm with the new isRangeVisible function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123544/diff/8-9/

Comment 217

[Brad Werth [:bradwerth]]

Comment on attachment 8860170 [details]
Bug 1302470 Part 6: Properly check for a frame's visibility, do not abuse isRangeVisible() for that purpose.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132194/diff/7-8/

Comment 218

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b7bd1dae9f15da963689c881345317a118c3d90

Comment 219

[Brad Werth [:bradwerth]]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #211)
> Make sure that if a caller wants to know a point from the offset, and really
> needs to have a correct answer, then that code flushes out layout before
> asking for the point?

Thanks for the info. This helped me find the right fix (I hope).

Comment 220

[Brad Werth [:bradwerth]]

I'm cautiously optimistic this is landable now. I'm going to try for it.

Comment 221

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33ce36cb798b
Part 1: Create a IsRangeRendered function to test range visibility in the display list. r=mstange,smaug
https://hg.mozilla.org/integration/autoland/rev/9dfcb2f9db7d
Part 2: Branch IsRangeVisible to delegate to IsRangeRendered when range is in viewport. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/97cf3bf30774
Part 3: Fix the case where HTML buttons need to generate display item children when doing opaque hit tests. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/c366831a7837
Part 4: Change nsDisplayList::GetOpaqueRegion non-opaque lists to build up a region from its children. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/20c8d9fcd818
Part 5: Connect up FinderHighlighter.jsm with the new isRangeVisible function. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/db3f44db75c5
Part 6: Properly check for a frame's visibility, do not abuse isRangeVisible() for that purpose. r=mikedeboer

Comment 222

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/33ce36cb798b
https://hg.mozilla.org/mozilla-central/rev/9dfcb2f9db7d
https://hg.mozilla.org/mozilla-central/rev/97cf3bf30774
https://hg.mozilla.org/mozilla-central/rev/c366831a7837
https://hg.mozilla.org/mozilla-central/rev/20c8d9fcd818
https://hg.mozilla.org/mozilla-central/rev/db3f44db75c5

Comment 223

[Timea Zsoldos [:zstimi/tzsoldos], Desktop Release QA]

I managed to reproduced the following testcase, from comment 130 using Firefox, on Windows 8.1 x64:
> 1) Navigate to http://searchfox.org .
> 2) Search for the word "welcome". Note that the only result is highlighted.
> 3) Scroll down so that the text disappears under the fixed position div at
> the top. The highlighted text will float over the fixed position div. 
I can confirm this part of issue is fixed, I verified using Firefox 58.0b3 on Win 8.1 x64, Mac OS X 10.12.6 and Ubuntu 14.04 x64.
Please let me known if this is enough for mark status on 58 as verified?

Comment 224

[Brad Werth [:bradwerth]]

Yes, fixed for 58.