find toolbar should highlight better/dim rest of page
Categories
(Toolkit :: Find Toolbar, enhancement, P1)
Tracking
()
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
Updated•17 years ago
|
Updated•16 years ago
|
Comment 1•16 years ago
|
||
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•15 years ago
|
||
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•11 years ago
|
||
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.)
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 6•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Comment 7•11 years ago
|
||
(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?
Assignee | ||
Comment 8•11 years ago
|
||
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 :)
Assignee | ||
Updated•11 years ago
|
Comment 9•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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.
Assignee | ||
Comment 12•11 years ago
|
||
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!
Updated•11 years ago
|
Comment 13•11 years ago
|
||
Heh, and then you bitrotted yourself again with c36f479a9755 (not too badly - fixed locally). Usually it's me doing this to myself ;)
Comment 14•11 years ago
|
||
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•11 years ago
|
||
And another positioning issue (same page) - this time the text in the highlight doesn't line up vertically.
Assignee | ||
Comment 16•11 years ago
|
||
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•11 years ago
|
||
Not playing well with text that wraps.
Comment 18•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
(In reply to Luís Miguel [:Quicksaver] from comment #21) I wondered about this approach too - seems worth exploring.
Assignee | ||
Comment 23•11 years ago
|
||
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?
Assignee | ||
Comment 24•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
(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.
Updated•11 years ago
|
Comment 31•11 years ago
|
||
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)
Updated•11 years ago
|
Updated•10 years ago
|
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•10 years ago
|
||
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.
Assignee | ||
Comment 34•10 years ago
|
||
(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...
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 35•10 years ago
|
||
Assignee | ||
Comment 36•10 years ago
|
||
Assignee | ||
Comment 37•10 years ago
|
||
Test document used to produce the two screenshots I attached before.
Assignee | ||
Comment 38•10 years ago
|
||
(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.
Assignee | ||
Comment 39•10 years ago
|
||
(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.
Assignee | ||
Comment 41•10 years ago
|
||
(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.
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 44•8 years ago
|
||
Assignee | ||
Comment 45•8 years ago
|
||
Assignee | ||
Comment 46•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Comment 47•8 years ago
|
||
(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.)
Assignee | ||
Comment 48•8 years ago
|
||
(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•8 years ago
|
||
(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•8 years ago
|
||
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.
Assignee | ||
Comment 51•8 years ago
|
||
(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•8 years ago
|
||
(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!
Updated•8 years ago
|
Assignee | ||
Comment 53•8 years ago
|
||
Updated•8 years ago
|
Comment 54•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
Was there something else you planned on implementing before requesting review?
Assignee | ||
Comment 57•8 years ago
|
||
(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•8 years ago
|
||
Cool, I like the idea of stubbing out insertAnonymousContent. Looking forward to final set of patches.
Assignee | ||
Comment 59•8 years ago
|
||
(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.
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 60•8 years ago
|
||
Note: I changed the order of the patches!
Assignee | ||
Comment 61•8 years ago
|
||
Assignee | ||
Comment 62•8 years ago
|
||
Assignee | ||
Comment 63•8 years ago
|
||
Whilst I'm working on the tests, I thought you might find the time to review this Jared?
Comment 64•8 years ago
|
||
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.
Assignee | ||
Comment 65•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Comment 66•8 years ago
|
||
Thanks I can review. Can you NI me with a screenshot?
Updated•8 years ago
|
Comment 67•8 years ago
|
||
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?
Updated•8 years ago
|
Comment 68•8 years ago
|
||
(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•8 years ago
|
||
a11y+ Thanks! This looks strictly better than what we have.
Assignee | ||
Comment 71•8 years ago
|
||
Fixed a few small issues found whilst writing the test. Carrying over r=jaws.
Comment 72•8 years ago
|
||
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.
Assignee | ||
Comment 73•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=66f82d3effd3
Assignee | ||
Comment 74•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=040f82e35285
Assignee | ||
Comment 75•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=34e371169ce7
Assignee | ||
Comment 76•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=35cfe9f4712e
Assignee | ||
Comment 77•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=02a646e0bf70
Assignee | ||
Comment 78•8 years ago
|
||
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.
Assignee | ||
Comment 79•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9be2cc46d142
Assignee | ||
Comment 80•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e92f82c09b3
Assignee | ||
Comment 81•8 years ago
|
||
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
Assignee | ||
Comment 82•8 years ago
|
||
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
Assignee | ||
Comment 83•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=85e72a8ea4e2
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
Assignee | ||
Comment 85•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cc8a2943a0c
Assignee | ||
Comment 86•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Comment 87•8 years ago
|
||
bugherder |
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•8 years ago
|
||
I think this is release note worthy, it's really awesome.
Comment 89•8 years ago
|
||
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•8 years ago
|
||
(In reply to Marco Castelluccio [:marco] from comment #89) > Should I file a new bug? I filed bug 1279685.
Updated•8 years ago
|
Comment 91•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
(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.
Assignee | ||
Comment 94•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
Screenshot demonstrating the issue of the highlight decreasing the readability of the surrounding text.
Comment 98•8 years ago
|
||
Is there a pref to disable this locally?
Comment 99•8 years ago
|
||
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•8 years ago
|
||
[Tracking Requested - why for this release]: There are currently about 23 regression bugs nominated for 50?, easier to track things from this bug.
Updated•8 years ago
|
Comment 101•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
(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•8 years ago
|
||
(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•8 years ago
|
||
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•8 years ago
|
||
(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•8 years ago
|
||
Hi :mikedeboer, Is the new find tool bar still only enabled in nightly and disabled in aurora now?
Assignee | ||
Comment 108•8 years ago
|
||
(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•8 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
It's still Nightly-only.
Comment 113•4 years ago
|
||
Has dim ever happened? I see this is "RESOLVED FIXED" but when I search, the page does not dim
Comment 114•4 years ago
|
||
oh, nevermind, I learn that I must manually toggle findbar.modalHighlight to true. Why isn't this enabled by default?!
Comment 115•4 years ago
|
||
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•4 years ago
|
||
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•4 years ago
|
||
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.
Description
•