384458 - find toolbar should highlight better/dim rest of page

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, enhancement, P1)

Description

[Judson]

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

Comment 1

[Gordon Mei]

Attachment: Example: Safari find-in-page dim and highlight

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.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 4

[Colby Russell :crussell (no longer Mozilla-ing)]

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.)

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: animated highlighting and page dim during fayt

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.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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?

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.1: animated highlighting and page dim during fayt

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 :)

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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?

Comment 10

[Luís Miguel [:quicksaver] (not reading bugmail)]

(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.

Comment 11

[Luís Miguel [:quicksaver] (not reading bugmail)]

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.

Comment 12

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.2: animated highlighting and page dim during fayt

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!

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Heh, and then you bitrotted yourself again with c36f479a9755 (not too badly - fixed locally). Usually it's me doing this to myself ;)

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Screenshot 1 - green highlight shown

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.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Screenshot 2 - poor vertical positioning

And another positioning issue (same page) - this time the text in the highlight doesn't line up vertically.

Comment 16

[Mike de Boer [:mikedeboer]]

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!

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Screenshot 3 - wrapping

Not playing well with text that wraps.

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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 :\

Comment 20

[(no longer active)]

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.

Comment 21

[Luís Miguel [:quicksaver] (not reading bugmail)]

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.

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Luís Miguel [:Quicksaver] from comment #21)

I wondered about this approach too - seems worth exploring.

Comment 23

[Mike de Boer [:mikedeboer]]

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?

Comment 24

[Mike de Boer [:mikedeboer]]

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].

Comment 25

[Luís Miguel [:quicksaver] (not reading bugmail)]

(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...

Comment 26

[Luís Miguel [:quicksaver] (not reading bugmail)]

(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.

Comment 27

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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]).

Comment 28

[Luís Miguel [:quicksaver] (not reading bugmail)]

(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.

Comment 31

[Luís Miguel [:quicksaver] (not reading bugmail)]

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)

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 33

[Luís Miguel [:quicksaver] (not reading bugmail)]

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.

Comment 34

[Mike de Boer [:mikedeboer]]

(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...

Comment 35

[Mike de Boer [:mikedeboer]]

Attachment: Current highlight popover crossing window borders

Comment 36

[Mike de Boer [:mikedeboer]]

Attachment: Current highlight popover crossing on transformed text

Comment 37

[Mike de Boer [:mikedeboer]]

Attachment: finddemo.html

Test document used to produce the two screenshots I attached before.

Comment 38

[Mike de Boer [:mikedeboer]]

(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.

Comment 39

[Mike de Boer [:mikedeboer]]

(In reply to Mike de Boer [:mikedeboer] from comment #38)
> Please check out attachment 8422302 [details]

I meant attachment 8422295 [details].

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 41

[Mike de Boer [:mikedeboer]]

(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.

Comment 44

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: make the background color of the SELECTION_ATTENTION range coloring fully transparent

Comment 45

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: move the highlighting code to its own module

Comment 46

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3: implement modal highlighting using the AnonymousContent API and SVG masks

Comment 47

[Luís Miguel [:quicksaver] (not reading bugmail)]

(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.)

Comment 48

[Mike de Boer [:mikedeboer]]

(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...

Comment 49

[Luís Miguel [:quicksaver] (not reading bugmail)]

(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 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

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.

Comment 51

[Mike de Boer [:mikedeboer]]

(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

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

(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!

Comment 53

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4: more sensible default pref values and capture 'Highlight All' in a pref too

Comment 54

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 55

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 56

[Jared Wein [:jaws] (please needinfo? me)]

Was there something else you planned on implementing before requesting review?

Comment 57

[Mike de Boer [:mikedeboer]]

(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.

Comment 58

[Jared Wein [:jaws] (please needinfo? me)]

Cool, I like the idea of stubbing out insertAnonymousContent. Looking forward to final set of patches.

Comment 59

[Mike de Boer [:mikedeboer]]

(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.

Comment 60

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.1: more sensible default pref values and capture 'Highlight All' in a pref too

Note: I changed the order of the patches!

Comment 61

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.1: move the highlighting code to its own module

Comment 62

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4.1: implement modal highlighting using the AnonymousContent API and SVG masks

Comment 63

[Mike de Boer [:mikedeboer]]

Whilst I'm working on the tests, I thought you might find the time to review this Jared?

Comment 64

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

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.

Comment 65

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4.2: implement modal highlighting using the AnonymousContent API and SVG masks

Comment 66

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks I can review. Can you NI me with a screenshot?

Comment 67

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 68

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to David Bolter [:davidb] from comment #66)
> Thanks I can review. Can you NI me with a screenshot?

https://vimeo.com/167239422

Comment 69

[David Bolter [:davidb] (NeedInfo me for attention)]

a11y+

Thanks! This looks strictly better than what we have.

Comment 70

[Mike de Boer [:mikedeboer]]

Attachment: Patch 5: test it

Finally!

Comment 71

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4.3: implement modal highlighting using the AnonymousContent API and SVG masks

Fixed a few small issues found whilst writing the test. Carrying over r=jaws.

Comment 72

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 73

[Mike de Boer [:mikedeboer]]

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

Comment 74

[Mike de Boer [:mikedeboer]]

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

Comment 75

[Mike de Boer [:mikedeboer]]

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

Comment 76

[Mike de Boer [:mikedeboer]]

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

Comment 77

[Mike de Boer [:mikedeboer]]

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

Comment 78

[Mike de Boer [:mikedeboer]]

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.

Comment 79

[Mike de Boer [:mikedeboer]]

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

Comment 80

[Mike de Boer [:mikedeboer]]

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

Comment 81

[Mike de Boer [:mikedeboer]]

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

Comment 82

[Mike de Boer [:mikedeboer]]

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

Comment 83

[Mike de Boer [:mikedeboer]]

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

Comment 84

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

Backed those out in https://hg.mozilla.org/integration/fx-team/rev/d03332bc9097 for frequent failures https://hg.mozilla.org/integration/fx-team/rev/d03332bc9097

Comment 85

[Mike de Boer [:mikedeboer]]

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

Comment 86

[Mike de Boer [:mikedeboer]]

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

Comment 87

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0d05e6a1bdc2
https://hg.mozilla.org/mozilla-central/rev/de5d300e4687
https://hg.mozilla.org/mozilla-central/rev/b94fedbf48b1
https://hg.mozilla.org/mozilla-central/rev/73176395400e
https://hg.mozilla.org/mozilla-central/rev/1c86ba5d7a5b

Comment 88

[Marco Castelluccio [:marco]]

I think this is release note worthy, it's really awesome.

Comment 89

[Marco Castelluccio [:marco]]

Attachment: Schermata del 2016-06-11 01-55-12.png

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?

Comment 90

[Marco Castelluccio [:marco]]

(In reply to Marco Castelluccio [:marco] from comment #89)
> Should I file a new bug?

I filed bug 1279685.

Comment 91

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 92

[Sylvestre Ledru [:Sylvestre]]

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/

Comment 93

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 94

[Mike de Boer [:mikedeboer]]

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.

Comment 95

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

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).

Comment 96

[The 8472]

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.

Comment 97

[The 8472]

Attachment: highlight readability.png

Screenshot demonstrating the issue of the highlight decreasing the readability of the surrounding text.

Comment 98

[Jeff Muizelaar [:jrmuizel]]

Is there a pref to disable this locally?

Comment 99

[B.J. Herbison]

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.)

Comment 100

[Marcia Knous [:marcia]]

[Tracking Requested - why for this release]: There are currently about 23 regression bugs nominated for 50?, easier to track things from this bug.

Comment 101

[Avi Halachmi (:avih)]

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/

Comment 102

[Nicholas Nethercote [inactive]]

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.

Comment 103

[B.J. Herbison]

(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).

Comment 104

[Lily Kim]

(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?

Comment 105

[The 8472]

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.

Comment 106

[avada]

(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.

Comment 107

[Gerry Chang [:gchang]]

Hi :mikedeboer,
Is the new find tool bar still only enabled in nightly and disabled in aurora now?

Comment 108

[Mike de Boer [:mikedeboer]]

(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.

Comment 109

[Ricardo]

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.

Comment 110

[Liz Henry (:lizzard)]

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.

Comment 111

[B.J. Herbison]

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.

Comment 112

[Ryan VanderMeulen [:RyanVM]]

It's still Nightly-only.

Comment 113

[mailinglists35]

Has dim ever happened? I see this is "RESOLVED FIXED" but when I search, the page does not dim

Comment 114

[mailinglists35]

oh, nevermind, I learn that I must manually toggle findbar.modalHighlight to true. Why isn't this enabled by default?!

Comment 115

[B.J. Herbison]

I just toggled findbar.modalHighlight and restarted and I don't see the page dim of the rest of the page, so maybe that aspect of this change has been removed.

I don't have any inside information as to why this project hasn't progressed, but the various problems described in the dependencies on this bug may be the reason.

Comment 116

[mailinglists35]

it sort of works for me in firefox stable 82.0 (64-bit) - sometimes the page brightens instead of darkening, other times it quickly brightens then darkens or viceversa - but most of the times it correctly darkens

but it does not work at all in nightly 84.0a1 (2020-10-21) (64-bit)

Comment 117

[mailinglists35]

it also sometimes works in nightly.
but anyway now I understand why it is disabled = because it is buggy.
what I don't understand is why is this fixed resolved when in fact it is not.