Closed Bug 384458 Opened 13 years ago Closed 4 years ago

find toolbar should highlight better/dim rest of page

Categories

(Toolkit :: Find Toolbar, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
relnote-firefox --- -
firefox50 + fixed

People

(Reporter: cohesion, Assigned: mikedeboer)

References

(Depends on 19 open bugs, Blocks 3 open bugs)

Details

(Keywords: user-doc-needed, Whiteboard: [parity-safari] p=0)

Attachments

(10 files, 12 obsolete files)

214.67 KB, image/png
Details
190.78 KB, image/png
Details
719 bytes, text/html
Details
3.36 KB, patch
masayuki
: review+
masayuki
: feedback+
Details | Diff | Splinter Review
16.49 KB, patch
jaws
: review+
Details | Diff | Splinter Review
37.96 KB, patch
jaws
: review+
Details | Diff | Splinter Review
7.12 KB, patch
jaws
: review+
Details | Diff | Splinter Review
40.75 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
1.58 MB, image/png
Details
16.78 KB, image/png
Details
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4

The find toolbar should highlight the text in a more obvious way. Possibly dimming the rest of the page as well (like Safari).

Reproducible: Always

Steps to Reproduce:
1. Search for a term
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: parity-safari
Product: Firefox → Toolkit
Whiteboard: parity-safari → [parity-safari]
Would this be easy to implement in Firefox Mac?  (Also, the screenshot doesn't capture the one-time pulsating highlight box behind the text each time the 'find next' button is clicked.  This all makes it much easier to find what's highlighted on the page.
I had a look out how this is implemented in Safari. It basically just covers the content window with a semi-transparent window. We should be able to get this same effect on all platforms except for X11 when used without a compositing manager. In that situation I think it's fair to just not use the effect.
Duplicate of this bug: 860689
In Firefox, at least, that wouldn't be necessary since the tabbrowser's anonymous browsers are now contained in a stack (class="browserStack"), allowing anyone to overlay arbitrary stuff to be drawn on top of content, e.g., the devtools highlighter.  ("Content" includes scrollbars, FWIW.  The same approach can be used to fix bug 259640.)
Duplicate of this bug: 404515
Depends on: 257061
Assignee: nobody → mdeboer
Dão/ Blair, this is my first rather complete shot at it. I'm confident enough about the concept now to show it to you ;)

The downside of this approach is that you can't scroll anymore while searching in pages that are long enough to be scrolled, due to the overlay. I'd love to know if you have a creative idea to work around this!(?)

Please check this out when you have time, there's no rush at all. Again, a spare time project.
Attachment #803121 - Flags: feedback?(dao)
Attachment #803121 - Flags: feedback?(bmcbride)
(In reply to Mike de Boer [:mikedeboer] from comment #6)
> The downside of this approach is that you can't scroll anymore while
> searching in pages that are long enough to be scrolled, due to the overlay.
> I'd love to know if you have a creative idea to work around this!(?)

That sounds like a deal-breaker to me :\

Does adding mousethrough="always" to the overlay element help?
IRC explains it all:

[10:12:11] <mikedeboer> Unfocused: I wanna kiss you.
[10:12:32] <mikedeboer> Unfocused: mousethrough="always" works like a charm!
[10:12:42] <Unfocused> mikedeboer: i'm suddenly thankful you won't be in Santa Clara for the Summit
[10:12:50] <Unfocused> ah, cool :)
[10:13:20] <mikedeboer> lol :)
Attachment #803121 - Attachment is obsolete: true
Attachment #803121 - Flags: feedback?(dao)
Attachment #803121 - Flags: feedback?(bmcbride)
Attachment #803604 - Flags: feedback?(dao)
Attachment #803604 - Flags: feedback?(bmcbride)
Status: NEW → ASSIGNED
Comment on attachment 803604 [details] [diff] [review]
Patch v1.1: animated highlighting and page dim during fayt

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

This is pretty badly bitrotten :\ So haven't been able to actually try using it yet.

Open questions:
* How does this work with screen readers?
* Does mousethrough affect touch input as well? 
* How about keyboard control/focus?

::: toolkit/content/widgets/findbar.xml
@@ +2002,5 @@
> +            node = node.parentNode;
> +          let style = node.ownerDocument.defaultView.getComputedStyle(node, "");
> +          return {
> +            fontFamily: style.fontFamily,
> +            fontSize: style.fontSize

font-weight? font-variant? font-stretch? font-size-adjust? font-kerning? text-shadow? text-decoration? -moz-text-decoration-*? text-transform? text-orientation? -moz-text-size-adjust?

Full page zoom? (ie, different device-to-CSS pixel ratio for this browser than what chrome has)

Will have to figure out how close we want the highlighted text to match.

@@ +2020,5 @@
> +          // a stack.
> +          if (stack.localName != "stack") {
> +            stack = this.browser.parentNode.insertBefore(document.createElementNS(
> +              kXULNS, "stack"), this.browser);
> +            stack.appendChild(this.browser);

Bit worried about how dynamically adding/removing a stack will affect other code. Might be best to just always have a stack as part of the <browser> binding - assuming there's no performance impact of doing that. As a bonus, that might also so this automagically work with all Toolkit apps (if they enable it).

@@ +2198,5 @@
> +          }
> +        ]]></body>
> +      </method>
> +
> +      <method name="_updateModalHighlightPos">

How well does this perform on lower-end machines?
Attachment #803604 - Flags: feedback?(bmcbride) → feedback+
(In reply to Blair McBride [:Unfocused] from comment #9)
> @@ +2020,5 @@
> > +          // a stack.
> > +          if (stack.localName != "stack") {
> > +            stack = this.browser.parentNode.insertBefore(document.createElementNS(
> > +              kXULNS, "stack"), this.browser);
> > +            stack.appendChild(this.browser);
> 
> Bit worried about how dynamically adding/removing a stack will affect other
> code. Might be best to just always have a stack as part of the <browser>
> binding - assuming there's no performance impact of doing that. As a bonus,
> that might also so this automagically work with all Toolkit apps (if they
> enable it).

As a little heads up, every time I tried physically moving a browser element, it seemed to trigger a content reload. My OmniSidebar add-on is a good example of this, every time you click the dock/undock button, the sidebar's browser element is moved and it triggers a content reload. This, of course, is only relative chrome content; I've never tried moving webpage browser elements (actually this is a lie, I think I've tried once but it threw errors all around the console so I quickly gave up on it, I don't remember exactly what I did though), or the browser element in the view source windows for that matter, which aren't contained in a stack, so I'm not sure if this would also apply to them.
Also (I'm sorry, I forgot when writing the previous comment), 

>          // Insert the main highlight box right after the browser
>          if (this.browser.nextSibling)
>            stack.insertBefore(this._modalHighlightContainer, this.browser.nextSibling);
>          else
>            stack.appendChild(this._modalHighlightContainer);

Is this necessary? If .nextSibling is null, insertBefore() will just act as appendChild() anyway and insert the node in the end as the last child. It just seems like an unnecessary check, that's all.
Rebased on top of bug 666816, added preliminary support for fullZoom correction.

Blair, this one can be tried locally. Please make sure to apply on top of the latest patch in bug 257061!
Attachment #803604 - Attachment is obsolete: true
Attachment #803604 - Flags: feedback?(dao)
Attachment #807188 - Flags: review?(bmcbride)
Heh, and then you bitrotted yourself again with c36f479a9755 (not too badly - fixed locally). Usually it's me doing this to myself ;)
Attached image Screenshot 1 - green highlight shown (obsolete) —
The positioning/site can be a little off. Here's an example from cnn.com (my universal test page, because it's so good at breaking things) where the green highlight from the normal find is visible behind the new popout-highlight.
Attached image Screenshot 2 - poor vertical positioning (obsolete) —
And another positioning issue (same page) - this time the text in the highlight doesn't line up vertically.
Awesome feedback! Looks like I will adopt 'your' cnn.com as a test site for this :)

This sure confirms that I need to copy way more font properties then I do currently (just like you mentioned before in your feedback on the patch).

Stay tuned for updates!
Attached image Screenshot 3 - wrapping (obsolete) —
Not playing well with text that wraps.
Comment on attachment 807188 [details] [diff] [review]
Patch v1.2: animated highlighting and page dim during fayt

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

::: browser/themes/osx/browser.css
@@ +2172,5 @@
> +  transition-timing-function: linear;
> +}
> +
> +.findbar-modalHighlight-container[outline-fayt] {
> +  background-color: rgba(0,0,0,.1);

IMO, this is far too subtle.

::: toolkit/content/widgets/findbar.xml
@@ +1429,5 @@
> +                highlightModalOutlineText.style.fontSize = correctSize(aData.fontStyle.fontSize);
> +              }
> +
> +              if (aData.findAgain)
> +                this._modalHighlightContainer.removeAttribute("outline-fayt");

(As mentioned on IRC) I originally thought this was a bug. Removing the dim when moving between matches feels odd to me. Is this what Safari does or something?

Similarly, it feels a bit disorienting when you're typing in the find field, the page is dimmed, and then suddenly it's not - because there suddenly isn't a match. I dunno, that may be a good thing - it certainly got my attention... just maybe too much.

@@ +1431,5 @@
> +
> +              if (aData.findAgain)
> +                this._modalHighlightContainer.removeAttribute("outline-fayt");
> +              else
> +                this._modalHighlightContainer.setAttribute("outline-fayt", "true");

This is an odd attribute name.

@@ +1443,5 @@
> +                if (outline.hasAttribute("grow"))
> +                  return;
> +
> +                window.mozRequestAnimationFrame(() => {
> +                  outline.setAttribute("grow", true);

Hm, this pop-in effect (which is nice) make it so when you move to the previous/next match, the old-style highlight is visible for a fraction of a second.

::: toolkit/modules/Finder.jsm
@@ +157,5 @@
> +  hideModalHighlight: function(aClear) {
> +    if (aClear)
> +      this.clearModalHighlight();
> +    for (let l of this._listeners)
> +      l.onModalHighlightResult("hide");

Ditto with the other bug - should be in a try/catch so one listener can't break everything/prevent other listeners from being notified.

Using the listeners wasn't a common thing before these patches - now it is. Seems worth it to refactor this out to a callListeners(method, ...args) method.

@@ +165,5 @@
> +    for (let l of this._listeners)
> +      l.onModalHighlightResult("clear");
> +  },
> +
> +  updateModalHighlight: function(res, aFindPrevious, aFindAgain) {

Bit of redundancy between this method and _modalHighlight.

@@ +173,5 @@
> +      Ci.nsITypeAheadFind.FIND_WRAPPED,
> +      Ci.nsITypeAheadFind.FIND_PENDING,
> +      Ci.nsITypeAheadFind.FIND_FOUND
> +    ];
> +    if (kValidStates.indexOf(res) != -1 && foundRange) {

Wouldn't it be easier to just see if the result was FIND_NOTFOUND?

@@ +653,5 @@
>  
> +  _getRangeContentArray: function(aRange) {
> +    let content = aRange.cloneContents();
> +    let textContent = [];
> +    for (let t, i = 0, l = content.childNodes.length; i < l; ++i) {

for-of

@@ +655,5 @@
> +    let content = aRange.cloneContents();
> +    let textContent = [];
> +    for (let t, i = 0, l = content.childNodes.length; i < l; ++i) {
> +      t = content.childNodes[i].textContent || content.childNodes[i].nodeValue;
> +      if (t && t.trim()) {

This lets you search for whitespace, and its show it using the old-style highlight... which is a bit odd.
Attachment #807188 - Flags: review?(bmcbride) → feedback+
(In reply to Blair McBride [:Unfocused] from comment #9)
> font-weight? font-variant? font-stretch? font-size-adjust? font-kerning?
> text-shadow? text-decoration? -moz-text-decoration-*? text-transform?
> text-orientation? -moz-text-size-adjust?

Forgot to also add: @font-face

Which is likely to be both very difficult to support, and may actually introduce a security risk if using it in chrome code :\
Another fun challenge, handle content reflows because of dynamic modifications to the DOM, resizing the window, changing the viewport size because of a change in the chrome (such as an infobar showing up), etc.
Currently, if I'm reading the code correctly, it is copying the ranges contents into the modal highlight box. Have you considered making the modal box "empty"? As in, only have the borders/pretty 3D effect, no background or text content of its own, showing the actual highlighted webpage text "in" it; basically just add the pretty borders and 3D effect to the actual highlighted text ranges, so it only seems like it pops out.

In theory, this would reduce the amount of work (and possible issues like in comment 19) the code does, by not needing to copy all the properties from the source text. Also in wrapping lines (comment 17), by iterating through all the items returned from getClientRects() and not just the first one, it wouldn't need to figure out exactly where is the text split between lines, and the boxes could be added directly as if they were separate highlights without a problem, although I'm guessing this isn't a very critical issue with the current approach anyway.

The drawbacks I can see to this is the default highlights color would have to be changed, from the current pink/magenta/whatever it is, to match the box's color, and it needs to ensure this color is used in the background and not foreground (high contrast dark pages with white text usually switch light highlight background and text colors). And of course, the "dim" filter would need to have "holes" in the highlighted text areas, so those aren't dimmed inside the modal boxes.

It's just a different approach, has its drawbacks of course. I'd like to know your opinion on it.
(In reply to Luís Miguel [:Quicksaver] from comment #21)

I wondered about this approach too - seems worth exploring.
That approach is interesting indeed. The biggest drawback I see is indeed the page dimming, which can only be drawn on top of the ranges.
For the active yellow-colored highlight we could apply a bit of color math to get a yellow color that is bright enough to compensate when desaturated with the opaque black layer on top.
The white 'highlight all' boxes can not be saturated any further to stay white. Making 'holes' in the opaque black layer can only be achieved in the way I do it now.
Perhaps we can tell a range with background fill to be drawn on top of all other frames? Perhaps we can ask someone from Layout/ Gfx about this?
Flags: needinfo?(bmcbride)
Oh, and my assumptions that it's ranges that are drawn with a background color is wrong; it's selections. This also explains attachment 809112 [details].
(In reply to Mike de Boer [:mikedeboer] from comment #23)
> Perhaps we can tell a range with background fill to be drawn on top of all
> other frames? Perhaps we can ask someone from Layout/ Gfx about this?
I'm guessing this would be the optimal way to do it (applied to the selection, not the ranges of course), but I did think of a few alternatives, in case this isn't possible, that I didn't mention before because I don't know if they're even possible. And because they definitely require more processing work, they may not really be better approaches anyway.

> Making 'holes' in the opaque black layer can only be achieved in the
> way I do it now.
Maybe this can be achieved through an SVG or a canvas system, instead of simple elements directly over other elements? Sorry if it's a dumb question, my knowledge on either is practically non-existent; all I know about them is they were created pretty much for this "graphical stuff". (Would scrolling/frames/combo-of-both be a problem here?)

Another option could be to not make the container universally dimmed. Instead, draw the modal highlight nodes first, then fill the remaining screen space with individual dimmed nodes, based on the coordinate limits from the modal highlights. (Again, in theory, scrolling could be a problem here, causing performance issues if there's a lot of nodes and drawing going on.)

> For the active yellow-colored highlight we could apply a bit of color math
> to get a yellow color that is bright enough to compensate when desaturated
> with the opaque black layer on top.
> The white 'highlight all' boxes can not be saturated any further to stay
> white.
Is there such a thing as a mini-print-screen? Another alternative would be to just copy pixel by pixel the screen contents in the area occupied by the ranges and paste it over the dimmed container and add the pretty border effects, instead of copying the individual text and all its properties so it looks like the same thing.

Like I said, not really great alternatives, just throwing them out there...
(In reply to Luís Miguel [:Quicksaver] from comment #25)
> I'm guessing this would be the optimal way to do it (applied to the
> selection, not the ranges of course)

Actually, after a moment's thought, I take this back. The optimal way would IMO be the holes approach (however they would end up being applied). Together with "pointer-events: none;" in the container, this would allow text selection/full interaction with the webpage without un-dimming the page, and open the way to possibly replacing the current highlights style with these modal highlight boxes (with and without dimming the page); in this case, when the page is un-dimmed, the boxes would remain until the highlights are actually removed.
(In reply to Mike de Boer [:mikedeboer] from comment #24)
> Oh, and my assumptions that it's ranges that are drawn with a background
> color is wrong; it's selections. This also explains attachment 809112 [details]
> [details].

Yea, highlight-all uses selections of type SELECTION_FIND. The colors are determined by cross-platform LookAndFeel here:
https://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsXPLookAndFeel.cpp#603

But that can be overridden by the ui.textHighlightBackground and ui.textHighlightForeground prefs (takes a hex-string color or a color name). Not as good as using CSS (would need bug 256773 for that), but better than nothing.

The current found match, however, is just a normal selection (SELECTION_NORMAL). Presumably we could change that by introducing a new selection type (see bug 451833 for an example of doing this), so it could be colored differently.

I doubt we could make the selections a higher z-order than the dimming-element. Perhaps there could be a way to make the platform-side do the dimming itself, ignoring selection types - but I doubt we'd be able to style anything. So Luís' canvas/SVG suggestion sounds like the safe bet - but you're still going to have to be extra careful about getting the holes in the dim layer in the right position (ie, the issue in attachment 809003 [details]).
Flags: needinfo?(bmcbride)
(In reply to Blair McBride [:Unfocused] from comment #27)
> The current found match, however, is just a normal selection
> (SELECTION_NORMAL). Presumably we could change that by introducing a new
> selection type (see bug 451833 for an example of doing this), so it could be
> colored differently.

BTW, while the current found match is just a normal selection, it is already of a different color, as (somewhere) it sets nsISelectionController.SELECTION_ATTENTION to the selection controller. By default, I believe this color is also based on the LookAndFeel here: https://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsXPLookAndFeel.cpp#596

This color can also be overridden by preference ui.textSelectBackgroundAttention; there is no ui.textSelectForegroundAttention, it still uses ui.textSelectForeground if it is set, but I found it better not to mess with that one as to not disrupt actual text selection.

However, with "highlight all" turned on, this SELECTION_ATTENTION marker would sometimes be lost:

1) Open find bar and click "Highlight All"
2) Type something in the find bar

The current match will be a different color for a moment, then revert back to normal selection color after highlights are placed; by hitting F3 the current match always has the SELECTION_ATTENTION color. I avoid this in my add-on, by always setting the SELECTION_ATTENTION marker on the current match selection after highlighting; something like:

> var curSel = controller.getSelection(Ci.nsISelectionController.SELECTION_NORMAL);
> if(curSel.rangeCount == 1)
>   controller.setDisplaySelection(Ci.nsISelectionController.SELECTION_ATTENTION);

This way, (for me, Windows 7) normal text selection is blue, current match is green.

Although a new selection type for the current match would be most welcomed on developing side, as it would be easier to manipulate that way (which I happen to desperately want for my add-on actually! So I'm speaking against myself here), I don't think it's something very important to add at the moment, at least not just for the sake of having a different color, because that can be already (somewhat) controlled.
Blocks: 565552
Whiteboard: [parity-safari] → [parity-safari][feature] p=0
Bug 950073 already depends on bug 565552 (which this bug already blocks), which is for tracking the improvement of the whole "Find in Page" feature, not just its highlights component. So this doesn't need to block it directly, as it's implied by bug 565552. ;)

(apologies if my assumption is wrong, and please re-block if so)
No longer blocks: fxdesktopbacklog
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: [parity-safari][feature] p=0 → [parity-safari] p=0
Seems to me the Mac effect is trying to look like dimming the whole page and then drawing the matched content again, above the dimmed page.

Does it work in Safari if the matched text is in a block which is "-webkit-transform:rotate(45deg) scale(2)"?

Does the matched-text popout need to overlap the window frame or extend outside it when the matched text is at the very edge of the window?

How are you planning to keep the matched-text popout/cutout synced with the document layout? Do we even care about that? Are we making the dimmed window responsive to events like scrolling?
Hey Mike, I found something kinda cool for this: https://addons.mozilla.org/firefox/addon/fokus/

I haven't checked the code to see how it works (I might this weekend, but until then it'll be impossible for me), so I don't know if it's a method you may already have considered or not, but I think it's worth checking out at least.
(In reply to Luís Miguel [:Quicksaver] from comment #33)

Hi Luís, thanks for that link! I saw it before, but didn't think of `clearRect()` before... I'll experiment if that supports randomly cutting out multiple rects. If that works, than this might be a solid approach.

Of course I still need to answer Robert's questions...
Attachment #809003 - Attachment is obsolete: true
Attachment #809004 - Attachment is obsolete: true
Attachment #809112 - Attachment is obsolete: true
Attachment #344944 - Attachment is obsolete: true
Flags: needinfo?(mdeboer)
Attached file finddemo.html
Test document used to produce the two screenshots I attached before.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)
> Seems to me the Mac effect is trying to look like dimming the whole page and
> then drawing the matched content again, above the dimmed page.

Hence the method I used in attachment 807188 [details] [diff] [review], which does exactly that.

> Does it work in Safari if the matched text is in a block which is
> "-webkit-transform:rotate(45deg) scale(2)"?

Please check out attachment 8422298 [details], which demonstrates what Safari does. I didn't (yet) check how it behaves when it's animated (related to your last question).

> Does the matched-text popout need to overlap the window frame or extend
> outside it when the matched text is at the very edge of the window?

Please check out attachment 8422302 [details], which demonstrates what Safari does. The box is drawn outside the window, when the match falls off the visible area. This is nicer visually, because the box won't loose the curved borders. The white, cut-out rectangles of other matches are not drawn atop the window chrome.

> How are you planning to keep the matched-text popout/cutout synced with the
> document layout? Do we even care about that?

So animating text will be a problem and I think it's fine to not deal with that in this first iteration.

> Are we making the dimmed window responsive to events like scrolling?

Yes. Safari, for example, supports scrolling, but hides the dimmed layer on mouse gestures like click and pinch.
So it seems like scroll is the only event that requires support.
(In reply to Mike de Boer [:mikedeboer] from comment #38)
> Please check out attachment 8422302 [details]

I meant attachment 8422295 [details].
(In reply to Mike de Boer [:mikedeboer] from comment #38)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)
> > How are you planning to keep the matched-text popout/cutout synced with the
> > document layout? Do we even care about that?

Seems like scrolling's going to be the main issue here. Is it OK if the popup box lags behind scrolling a little bit? Because I can't think of a way to avoid that.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40)
> Seems like scrolling's going to be the main issue here. Is it OK if the
> popup box lags behind scrolling a little bit? Because I can't think of a way
> to avoid that.

I can't think of one either, but that's ok. In Safari the popup box doesn't lag behind, but we're doing a x-platform implementation here and the challenges/ limitations that come with it.
On top of that, the scroll-lag would be a perf optimization, which I think is fine to track in a follow-up.
Duplicate of this bug: 1020171
Duplicate of this bug: 552304
Priority: -- → P1
Blocks: 466740
Assignee: mdeboer → nobody
Status: ASSIGNED → NEW
Depends on: 1269677
Depends on: 1270174
Blocks: 1271782
Assignee: nobody → mdeboer
Attachment #807188 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8753999 - Flags: feedback?(jaws)
Attachment #8753998 - Flags: feedback?(masayuki)
(In reply to Mike de Boer [:mikedeboer] from comment #46)
> Created attachment 8754006 [details] [diff] [review]
> Patch 3: implement modal highlighting using the AnonymousContent API and SVG
> masks

Needs preference listeners to update findbar._useModalHighlight and FinderHighlighter._modal when the preference is changed in about:config, otherwise changing it won't affect current tabs with the findbar already initialized, no?

(I haven't seen the actual highlighting methods in detail yet, that's just something I noticed.)
(In reply to Luís Miguel [:quicksaver] from comment #47)
> Needs preference listeners to update findbar._useModalHighlight and
> FinderHighlighter._modal when the preference is changed in about:config,
> otherwise changing it won't affect current tabs with the findbar already
> initialized, no?

True, intentionally left out. I was thinking not to bother with runtime switching support... restarting your browser to toggle modes seems fine to me in this case...
(In reply to Mike de Boer [:mikedeboer] from comment #48)
> True, intentionally left out. I was thinking not to bother with runtime
> switching support... restarting your browser to toggle modes seems fine to
> me in this case...

Can't say I fully agree, since almost all Firefox preferences don't require restart; even things like "layout.css.devPixelsPerPx" toggle immediately. But it's not something I'd really oppose either if you don't plan to expose it in the options page anyway. ;)
Comment on attachment 8753998 [details] [diff] [review]
Patch 1: make the background color of the SELECTION_ATTENTION range coloring fully transparent

What are you asking me?

Anyway, I wonder, you should be careful about new color. Currently, we swap the selection foreground color and background color if the selection background color and the text's background color doesn't have sufficient contrast. So, if the new highlight's background is similar to the text's background color, it might be not easy to find the highlight.
Flags: needinfo?(mdeboer)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #50)
> Comment on attachment 8753998 [details] [diff] [review]
> Patch 1: make the background color of the SELECTION_ATTENTION range coloring
> fully transparent
> 
> What are you asking me?

Hum hum, yeah, not a lot of context here, now is there! Sorry.
What I'm doing here is making the selection color transparent in order to have it appear hidden without negatively impacting the find-in-page behavior - which would happen if I'd simply collapse the selection/ range.
What I'm asking you is whether my proposed solution is acceptable code-wise.
Don't worry about the visibility of the selection, because here's what it'll look like instead: https://vimeo.com/167239422
Flags: needinfo?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #51)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #50)
> > Comment on attachment 8753998 [details] [diff] [review]
> > Patch 1: make the background color of the SELECTION_ATTENTION range coloring
> > fully transparent
> > 
> > What are you asking me?
> 
> Hum hum, yeah, not a lot of context here, now is there! Sorry.
> What I'm doing here is making the selection color transparent in order to
> have it appear hidden without negatively impacting the find-in-page behavior
> - which would happen if I'd simply collapse the selection/ range.

I have any objections but perhaps, you should ask a11y staff, although, I don't know the person who you should ask this to.

> What I'm asking you is whether my proposed solution is acceptable code-wise.
> Don't worry about the visibility of the selection, because here's what it'll
> look like instead: https://vimeo.com/167239422

Looks great!
Attachment #8753998 - Flags: feedback?(masayuki) → feedback+
Comment on attachment 8754006 [details] [diff] [review]
Patch 3: implement modal highlighting using the AnonymousContent API and SVG masks

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

::: toolkit/modules/Finder.jsm
@@ +171,5 @@
>      this.highlighter.notifyFinished(aHighlight);
>      if (aHighlight) {
>        let result = found ? Ci.nsITypeAheadFind.FIND_FOUND
>                           : Ci.nsITypeAheadFind.FIND_NOTFOUND;
> +      this._notify(aWord, result, false, false, false, false);

This is a good example of why we shouldn't be adding another boolean argument to the _notify function. Since you need to add another argument in this patch, please change _notify to use an `options` argument that will allow named parameters.

::: toolkit/modules/FinderHighlighter.jsm
@@ +12,4 @@
>  Cu.import("resource://gre/modules/Task.jsm");
>  
> +const kHighlightIterationSizeMax = 100; // in iterations.
> +const kModalHighlightRepaintFreq = 20; // in ms.

I don't know what the `// in iterations` comment provides that the variable name doesn't already say.

Please rename kModalHighlightRepaintFreq to kModalHighlightRepaintFreqMS and drop the comment.

@@ +19,5 @@
> +const kFontPropsCamelCase = kFontPropsCSS.map(prop => {
> +  let parts = prop.split("-");
> +  return parts.shift() + parts.map(part => part.charAt(0).toUpperCase() + part.slice(1)).join("");
> +});
> +const kModalIdPrefix = "cedee4d0-74c5-4f2d-ab43-4d37c0f9d463";

Please add a comment above this explaining why we used the prefix.

@@ +24,5 @@
> +const kModalOutlineId = kModalIdPrefix + "-findbar-modalHighlight-outline";
> +const kModalStyle = `
> +.findbar-modalHighlight-outline {
> +  position: absolute;
> +  background: linear-gradient(to bottom,#f1ee00,#edcc00);

nit, spaces after commas. we only skip spaces in rgba or hsla value lists.

@@ +29,5 @@
> +  border: 1px solid #f5e600;
> +  border-radius: 3px;
> +  box-shadow: 0px 2px 3px rgba(0,0,0,.8);
> +  color: #000;
> +  margin: -3px 0 0 -3px;

Switch this to:
margin-top: -3px;
margin-inline-end: 0;
margin-bottom: 0;
margin-inline-start: -3px;

Same for the padding.

and then you can drop the -moz-locale-dir(rtl) rules below.

@@ +241,5 @@
> +   * @return {AnonymousContent}    Reference to the node inserted into the
> +   *                               CanvasFrame. It'll also be stored in the
> +   *                               `_modalHighlightOutline` member variable.
> +   */
> +  show: function(window = null) {

nit, please use the

show(window = null) {
  ...
}

function style.

@@ +533,5 @@
> +   */
> +  _repaintHighlightAllMask: function(window) {
> +    let document = window.document;
> +
> +    // Always remove the current mask and insert it a-fresh as a burst render.

can you explain what you mean by this?

@@ +589,5 @@
> +
> +  /**
> +   * Doing a full repaint each time a range is delivered by the highlight iterator
> +   * is way too costly, thus we pipe the frequency down to every
> +   * `kModalHighlightRepaintFreq` milliseconds.

20ms is what this constant is set to, which is just past 1 frame (16.67ms) and quite a bit below 2 frames (33.33ms). If this is very costly, can we drop a couple frames here without much noticeable effect? What happens if we have this set to 35ms or 50ms (3 frames)?

@@ +666,5 @@
> +    ];
> +    window.addEventListener("resize", this._highlightListeners[0]);
> +    window.addEventListener("DOMContentLoaded", this._highlightListeners[0]);
> +    window.addEventListener("mousedown", this._highlightListeners[1]);
> +    window.addEventListener("touchstart", this._highlightListeners[1]);

Should keypress be in here too, for Escape?
Attachment #8754006 - Flags: feedback?(jaws) → feedback+
Comment on attachment 8754344 [details] [diff] [review]
Patch 4: more sensible default pref values and capture 'Highlight All' in a pref too

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

::: toolkit/content/widgets/findbar.xml
@@ +506,5 @@
>  
>        <!--
>          - Turns highlight on or off.
>          - @param aHighlight (boolean)
>          -        Whether to turn the highlight on or off

Please add the javadoc for the new parameter.
Attachment #8754344 - Flags: feedback?(jaws) → feedback+
Was there something else you planned on implementing before requesting review?
Flags: needinfo?(mdeboer)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #56)
> Was there something else you planned on implementing before requesting
> review?

No not really; the idea was that if Masayuki and you think this approach is solid, I can answer your questions above and implement the suggested changes, this set of patches is the full implementation of this feature.
I intend to implement unit tests by stubbing the `insertAnonymousContent` function and inspecting the result in various scenarios that way.
Flags: needinfo?(mdeboer)
Cool, I like the idea of stubbing out insertAnonymousContent. Looking forward to final set of patches.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #54)
> can you explain what you mean by this?

I've change this comment to 'Always remove the current mask and insert it a-fresh, because we're not free to alter DOM nodes inside the CanvasFrame.'... Is that more clear?

> 20ms is what this constant is set to, which is just past 1 frame (16.67ms)
> and quite a bit below 2 frames (33.33ms). If this is very costly, can we
> drop a couple frames here without much noticeable effect? What happens if we
> have this set to 35ms or 50ms (3 frames)?

Well, it's only noticeable when we match more that we will draw in one iteration - 100 ranges at the moment - because then we re-render the mask with the updated mask.
However, since the highlightIterator already shortly sleeps after a 100 ranges, we could make this 10ms instead. I just want to be really careful not to cause any jank here.

> Should keypress be in here too, for Escape?

No, because the findbar itself already listens to and reacts to the keypress and during FAYT, the findbar input is also the focused element. Therefore I implemented the 'onFindbarClose' message to make sure that the modal highlight stuff hides whenever the findbar does in all scenarios.
Attachment #8753998 - Flags: review?(masayuki)
Note: I changed the order of the patches!
Attachment #8754344 - Attachment is obsolete: true
Attachment #8755852 - Flags: review?(jaws)
Attachment #8753999 - Attachment is obsolete: true
Attachment #8755853 - Flags: review?(jaws)
Whilst I'm working on the tests, I thought you might find the time to review this Jared?
Comment on attachment 8753998 [details] [diff] [review]
Patch 1: make the background color of the SELECTION_ATTENTION range coloring fully transparent

r=me if a11y team doesn't have any objection about this approach.
Attachment #8753998 - Flags: review?(masayuki) → review+
Attachment #8755854 - Attachment is obsolete: true
Attachment #8755854 - Flags: review?(jaws)
Attachment #8755861 - Flags: review?(jaws)
Flags: a11y-review?
Thanks I can review. Can you NI me with a screenshot?
Comment on attachment 8755853 [details] [diff] [review]
Patch 3.1: move the highlighting code to its own module

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

::: toolkit/modules/FinderHighlighter.jsm
@@ +77,5 @@
> +      this._abortHighlight = () => {
> +        this._abortHighlight = null;
> +        reject();
> +      };
> +      this.finder._getWindow().setTimeout(resolve, delay);

When this promise is resolved, should the _abortHightlight function get set to null? Right now it's only deleted if _abortHighlight is called.

@@ +95,5 @@
> +    window = window || this.finder._getWindow();
> +    let found = false;
> +    for (let i = 0; window.frames && i < window.frames.length; i++) {
> +      if (yield this.highlight(highlight, word, window.frames[i]))
> +        found = true;

nit, please surround one-line if-statements with brackets.

@@ +171,5 @@
> +   * @param aNode the node we want to check
> +   * @returns the first node in the parent chain that is editable,
> +   *          null if there is no such node
> +   */
> +  _getEditableNode: function(aNode) {

Can you switch to the 
_getEditableNode(aNode) {
 ...
}
style?
Attachment #8755853 - Flags: review?(jaws) → review+
(In reply to David Bolter [:davidb] from comment #66)
> Thanks I can review. Can you NI me with a screenshot?

https://vimeo.com/167239422
Flags: needinfo?(dbolter)
a11y+

Thanks! This looks strictly better than what we have.
Flags: needinfo?(dbolter)
Flags: a11y-review?
Flags: a11y-review+
Blocks: 1275891
Attached patch Patch 5: test itSplinter Review
Finally!
Attachment #8759566 - Flags: review?(jaws)
Fixed a few small issues found whilst writing the test. Carrying over r=jaws.
Attachment #8755861 - Attachment is obsolete: true
Attachment #8759572 - Flags: review+
Comment on attachment 8759566 [details] [diff] [review]
Patch 5: test it

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

The setTimeout business here where we delay evaluating the results until some amount of time, 500ms in this case, feels a bit odd, but I'm willing to live with it now.

We'll see over time just how many random failures this will generate, and you can rewrite the test and code at that point to notify observers or something like that to let the test code know that the find-in-page and highlighting code has completed.
Attachment #8759566 - Flags: review?(jaws) → review+
For those following along: I'm fighting unit tests right now. So a lot of code paths will change with this feature and part of that is due to a much needed refactor of the Finder.jsm class.

The failures I encountered were:
1) Tests expecting ranges to be added to a selection, not a modal HTML layer. I remedied this to have the `finder.modalHighlight` and `finder.highlightAll` set to `false` using 'prefs_general.js' for the existing tests and set them to the new values in the newly added tests using `SpecialPowers.pushPrefEnv()`.
2) Memory leaks. These were plain bugs, but quite hard to trace back. I found and fixed them all now, but this was the main reason for most of the try pushes above.
3) Platform-specific assertion failures. So one test runs fine on OSX, but fails consistently on Windows. I'm switching to work on my Linux and Windows boxes tomorrow, so expect these to be fixed this week.

This might sound odd, but I'm actually quite happy that this is taking a bit of time to get ready to land. Because this confirms - to a certain degree - that our test coverage for the findbar is quite extensive.
https://hg.mozilla.org/integration/fx-team/rev/5eabde153def61f4577dbcf7d3726dc1d0b0f5c5
Bug 384458 - part 1: make the background color of the SELECTION_ATTENTION range fully transparent when modal highlighting is on. r=masayuki

https://hg.mozilla.org/integration/fx-team/rev/091c5465acf79b6da8807e6a959417755016cae2
Bug 384458 - part 2: change the matches count timeout to 100ms, up the limit to a 1000 counts and capture the state of 'Highlight All' in a pref which will be TRUE by default in the browser. r=jaws

https://hg.mozilla.org/integration/fx-team/rev/3b025cbfbe3c0b24677b71c466ec1ce3bc9e2773
Bug 384458 - part 3: move the highlighting code to its own module. r=jaws

https://hg.mozilla.org/integration/fx-team/rev/ae71396d3be08f28b17b4cf4b2174184d353efee
Bug 384458 - part 4: implement modal highlighting using the AnonymousContent API and SVG masks. r=jaws

https://hg.mozilla.org/integration/fx-team/rev/63ec66da50faad032454a44d3af3124c35102b3e
Bug 384458 - part 5: add unit test coverage for the findbar modal highlight feature. r=jaws
https://hg.mozilla.org/integration/fx-team/rev/ded915ec401f6690c95486a68ac2546806aa3227
Bug 384458 - part 7: up the hard-coded match count limit from 100 to 1000. r=me
https://hg.mozilla.org/integration/fx-team/rev/0d05e6a1bdc206eddb08d4dc6e24a8e30801064f
Bug 384458 - part 1: make the background color of the SELECTION_ATTENTION range fully transparent when modal highlighting is on. r=masayuki

https://hg.mozilla.org/integration/fx-team/rev/de5d300e468729ae3dede732849298a6fe84b0dc
Bug 384458 - part 2: change the matches count timeout to 100ms, up the limit to a 1000 counts and capture the state of 'Highlight All' in a pref which will be TRUE by default in the browser. r=jaws

https://hg.mozilla.org/integration/fx-team/rev/b94fedbf48b115731b39dff47026714513338205
Bug 384458 - part 3: move the highlighting code to its own module. r=jaws

https://hg.mozilla.org/integration/fx-team/rev/73176395400e2a0e667e2501365e84ba76f355a6
Bug 384458 - part 4: implement modal highlighting using the AnonymousContent API and SVG masks. r=jaws

https://hg.mozilla.org/integration/fx-team/rev/1c86ba5d7a5b9cb16ef3ad77ff740688a923108a
Bug 384458 - part 5: add unit test coverage for the findbar modal highlight feature. r=jaws
Flags: needinfo?(mdeboer)
I think this is release note worthy, it's really awesome.
There is something weird happening on this page (http://www.ansa.it/) if you search for something that is hidden in the carousel at the end of the page.

It's hard to describe, so please see if you can reproduce.

Should I file a new bug?
Depends on: 1279652
Depends on: 1279681
Depends on: 1279682
Depends on: 1279683
Depends on: 1279684
Depends on: 1279685
(In reply to Marco Castelluccio [:marco] from comment #89)
> Should I file a new bug?

I filed bug 1279685.
Blocks: 1279692
Depends on: 1279695
No longer blocks: 1279692
Depends on: 1279692
Depends on: 1279704
Depends on: 1279707
Depends on: 1279708
Depends on: 1279710
No longer depends on: 1279708
Depends on: 1279711
No longer depends on: 1279711
Depends on: 1279715
Depends on: 1279717
Depends on: 1279719
Depends on: 1279721
Depends on: 1279723
Depends on: 1279742
Depends on: 1279751
Depends on: 1279752
It seems like this feature has a introduced a bunch of problems and the performance is not very good (the animation is janky and it increases checkerboarding noticeably). Perhaps we should turn it off until it works better?
Release Note Request (optional, but appreciated)
[Why is this notable]: New user facing feature
[Suggested wording]: Page search improved
[Links (documentation, blog post, etc)]: we should have a doc for this
More here:
http://www.ghacks.net/2016/06/11/firefox-page-search-improvements/
relnote-firefox: --- → ?
Keywords: user-doc-needed
Depends on: 1279802
Depends on: 1279803
(In reply to Jeff Muizelaar [:jrmuizel] from comment #91)
> It seems like this feature has a introduced a bunch of problems and the
> performance is not very good (the animation is janky and it increases
> checkerboarding noticeably). Perhaps we should turn it off until it works
> better?

This just landed in the beginning of the 50 Nightly cycle, so I think it would be better to keep it enabled at least for the next two weeks so we can gather more bug reports on any fallout from the change.
I agree. I've got a couple of improvements worked out together with Markus (:mstange) that should resolve a couple of issues, among which improve performance.
I intend to follow through here and attempt to resolve all blocking issues well before uplift. I am presuming many issues will be blocking, since this is a very user-facing feature. But that's all the better.
I can understand if you'd like to see this turned off because you don't like the feature, but that's a matter I can not influence.
I think Nightly is still usable for the intended audience and I hope that with my promise to be on point here is enough to keep things as they are for now.

Aside, I want to express my gratitude to all the enthusiastic folks who've been filing bugs during the weekend with such fantastic spirit. This is truly inspiring.
I think that we should be careful if this is enabled in release builds because like this fix giving big impact to users can give negative impact too if there are a lot of issues but for collecting bug reports, we should keep enabling this at least in Nightly builds (ideally, non-release builds).
I find the usability of the new highlighting to be suboptimal. The yellow border covers adjacent words/letters, reducing readability of the context, which can be important when hitting partial matches.

It basically makes finding things easier (which indeed was a problem before!), but then hampers the next step of reading around that match.
Screenshot demonstrating the issue of the highlight decreasing the readability of the surrounding text.
Depends on: 1279843
Depends on: 1279922
Depends on: 1280149
No longer depends on: 1279752
Is there a pref to disable this locally?
From Mike de Boer [:mikedeboer]:

> Setting the pref 'findbar.modalHighlight' to 'false' will turn off the feature
and restore the old one.

(Possibly only temporary feature switch until the bugs get ironed out in the new implementation.)
Depends on: 1280354
Depends on: 1280525
Depends on: 1280733
Depends on: 1280755
No longer depends on: 1280755
Depends on: 1280751
Depends on: 1280831
Depends on: 1280876
[Tracking Requested - why for this release]: There are currently about 23 regression bugs nominated for 50?, easier to track things from this bug.
Depends on: 1281421
Depends on: 1281462
Depends on: 1282070
Depends on: 1282036
Depends on: 1282720
Depends on: 1282752
Depends on: 1282766
I hope I haven't missed it and that it's not too late to ask, but has there been some discussion on whether or not this feature is/would-be desirable for most users?

I can say for myself that the previous highlight paradigm was definitely good enough for me. The highlight itself is visible enough all the time (unless you're visiting myspace pages with magenta background, I guess).

If I wanted to enhance the default page search (which I do), I'd add find markers around the scrollbar to get a sense of how many search results there are and where are they at the overall page.

The above is also the reason I use the findbar-tweaks[1] addon which does so (scrollbar markers) (off topic - it's partially broken right now with the changes of this bug).

Considering all the blockers this bug has now, would it be a good time to reconsider if it's worth the effort and potential complexity?


[1] https://addons.mozilla.org/en-US/firefox/addon/findbar-tweak/
Depends on: 1283041
Depends on: 1283042
Depends on: 1283078
Depends on: 1283440
Blocks: 1283472
FWIW: I don't mind the dimming so much, but I find the way the yellow box moves around -- particularly when (a) typing the search term, and (b) using ctrl-g to move between searches -- very jerky and distracting.
(In reply to Nicholas Nethercote [:njn] from comment #102)
> FWIW: I don't mind the dimming so much, but I find the way the yellow box
> moves around -- particularly when (a) typing the search term, and (b) using
> ctrl-g to move between searches -- very jerky and distracting.

That sounds like some user experience research is needed to see what combination of fading and/or shifting would annoy the fewest people, and possibly whether another color would work better (while keeping color blindness and other common visual problems in mind).
(In reply to B.J. Herbison from comment #103)
I agree. I personally like the dimming and the yellow box moving.

>and possibly whether another color would work better
Wouldn't it be possible to have your own custom color set, perhaps by an about:config tweak?
I think there are a bunch of conflicting goals wrt. color/dimming choices:

* suppress visual noise on the page so that highlights are easy to find spot on colorful, cluttered pages
* dimming should work on any page, regardless what its picks as text and background colors.
* maintain readability of the context. a match is useless without context
* maintain accessibility for colorblind or otherwise vision-impaired people

I think those goals will fail to be met on some pages if they choose some unusual color combinations

Dark-grey-on-black could become unreadable when dimmed.
If we used desaturating instead it would fail on colors of equal lightness but different hue.
Any entropy-preserving color transformation could hurt accessibility or fail to meet the visual-noise-reduction goal.

It might be better to temporarily override color/background/text effect styles on the whole page (and possibly other styles as long as they don't affect layout) to a combination of foreground/background colors that's known to fulfill all criteria.
Depends on: 1291278
Depends on: 1295539
Depends on: 1295540
Depends on: 1297543
Depends on: 1298323
Depends on: 1301684
Depends on: 1301953
Depends on: 1279708
Depends on: 1302035
Depends on: 1302049
(In reply to Avi Halachmi (:avih) from comment #101)
> The above is also the reason I use the findbar-tweaks[1] addon which does so
> (scrollbar markers) (off topic - it's partially broken right now with the
> changes of this bug).

That's a feature I also sorely miss. It can be a significant help in finding things. Especially if there are a lot of results.
Depends on: 1302207
No longer depends on: 1302207
Depends on: 1302756
Depends on: 1303207
See Also: → 1303248
Hi :mikedeboer,
Is the new find tool bar still only enabled in nightly and disabled in aurora now?
Flags: needinfo?(mdeboer)
(In reply to Gerry Chang [:gchang] from comment #107)
> Hi :mikedeboer,
> Is the new find tool bar still only enabled in nightly and disabled in
> aurora now?

Yes.
Flags: needinfo?(mdeboer)
Depends on: 1304292
Depends on: 1306320
No longer depends on: 1306320
Now we have new annoyances.

This:
- Highlight vanishes when find bar is closed. No older version I remember did that.

And this:
1. Enable "Highlight All" and type something to find. Good we have things highlighted.
2. Close find bar. Highlights vanish.
3a. F3 will go next word without highlights.
3b. Ctrl+F to open find bar. No highlight. We need to disable and re-enable "Highlight All".

Using "Dim the page" (findbar.modalHighlight) we have new issues:
- Darker background on black text reduces visibility of context. Usability issue.
- Zoomed text overlaps texts around. Then, if we search part of a word, the word could be entirely tapped.
- Possibly much more usability issues.
Depends on: 1316513
Depends on: 1316514
Depends on: 1316515
Depends on: 1316516
Depends on: 1309208
Depends on: 1328034
Removing the release note flag here. This meta bug was closed during the Firefox 50 timeframe and we're about to release 52. If specific dependent bugs get fixed, let's use the release note flag on those bugs.
Has this been released? I just tried Firefox beta 52 I saw the old find behavior, but with Nightly 54 I see the new find behavior.
It's still Nightly-only.
Depends on: 1356823
You need to log in before you can comment on or make changes to this bug.