The screenshot attached shows a minor issue where the forward button bleeds through on the bottom of the URL bar when the forward button is disabled and the user hovers their mouse over the back button. [This bug affects Windows and Mac OS X themes]

Comment on attachment 617322 [details] [diff] [review] Patch v1 when clicking the forward button such that forward will be disabled and moving the pointer over to the back button, this makes the forward button invisible too early

this also happens when the back-button is disabled and is even more apparent using a light lw-theme. since there aren't any favicons in the locationbar anymore, this bleedthrough has become very visible.

This causes an animated repaint on hover of the back button only when it's hidden. This is really sub-optimal and what the hell.

This already happens now when you just hover somewhere over the Location Bar.

(In reply to Mehmet Sahin from comment #9) > This already happens now when you just hover somewhere over the Location Bar. I meant the Australis theme.

This wfm. Note the 'manual' call in onUpdateCurrentBrowser, which is only there because otherwise, if you hover the back button and hit ctrl+tab and hit a tab that has a disabled forward button, it'll still show up (and won't transition into hiddenness until you take your mouse off the hovered back-button).

Comment on attachment 811393 [details] [diff] [review] hide conditional forward button permanently when not in use to prevent accidental glitches, Review of attachment 811393 [details] [diff] [review]: ----------------------------------------------------------------- I applied the patch and still see the same issue. See this screencast: http://screencast.com/t/BmmMmY1wO

Comment on attachment 811393 [details] [diff] [review] hide conditional forward button permanently when not in use to prevent accidental glitches, Review of attachment 811393 [details] [diff] [review]: ----------------------------------------------------------------- More importantly, the forward button disappears with this patch before the mouse leaves the area: http://screencast.com/t/vdJfDLndcu

Comment on attachment 811393 [details] [diff] [review] hide conditional forward button permanently when not in use to prevent accidental glitches, (In reply to Jared Wein [:jaws] from comment #13) > Comment on attachment 811393 [details] [diff] [review] > hide conditional forward button permanently when not in use to prevent > accidental glitches, > > Review of attachment 811393 [details] [diff] [review]: > ----------------------------------------------------------------- > > More importantly, the forward button disappears with this patch before the > mouse leaves the area: http://screencast.com/t/vdJfDLndcu I wrote and tested this patch against m-c, as that's what this was filed against, and it doesn't have either of these problems there. If things have changed on UX then obviously it might need changes if/when merged. Can you check if this works for you on m-c? I'll look into the UX branch issue.

Comment on attachment 811393 [details] [diff] [review] hide conditional forward button permanently when not in use to prevent accidental glitches, Ah, but it's a windows issue compared to OS X - it's broken on Windows m-c as well. I'll have a look.

Ahhh, there are multiple transitions on Windows, so it got hidden too early. Here's a new patch for m-c that's also tested on Windows. I've also switched to using classes rather than element.style so as to avoid breaking 3rd party themes.

For reference, this is what the patch looks like when merged on UX

I forgot that the style bits should only apply when the conditional forward button is in use, oops.

Comment on attachment 811487 [details] [diff] [review] hide conditional forward button permanently when not in use to prevent accidental glitches, >+function UpdateForwardButtonHiddenState(aEvent) { >+ if (aEvent && aEvent.propertyName != "opacity") { >+ return; >+ } >+ let forwardButton = document.getElementById("forward-button"); >+ let shouldBeVisible = gBrowser.webNavigation.canGoForward; >+ forwardButton.classList[shouldBeVisible ? "remove" : "add"]("fullydisabled"); >+} This code will run unexpectedly e.g. when a theme transitions the opacity on hover. >+ // Add a transition listener to the fwd button to permanently hide it if necessary: s/fwd/forward/ > @conditionalForwardWithUrlbar@:not(:hover) > #forward-button[disabled] { > opacity: 0; > } > >+@conditionalForwardWithUrlbar@ > #forward-button.fullydisabled { >+ visibility: hidden; >+} "fullydisabled" is pretty sketchy, as it's not clear how it differs from "disabled". E.g. one might wrongly think that "disabled" doesn't mean that clicks aren't consumed. I'd suggest "invisible" or even something more verbose like "moved-behind-urlbar". See also bug 845408 comment 23.

(In reply to Dão Gottwald [:dao] from comment #19) > Comment on attachment 811487 [details] [diff] [review] > hide conditional forward button permanently when not in use to prevent > accidental glitches, > > >+function UpdateForwardButtonHiddenState(aEvent) { > >+ if (aEvent && aEvent.propertyName != "opacity") { > >+ return; > >+ } > >+ let forwardButton = document.getElementById("forward-button"); > >+ let shouldBeVisible = gBrowser.webNavigation.canGoForward; > >+ forwardButton.classList[shouldBeVisible ? "remove" : "add"]("fullydisabled"); > >+} > > This code will run unexpectedly e.g. when a theme transitions the opacity on > hover. As long as all it does is toggle an attribute when that happens, I don't think that is a problem. I don't think a simple, pure CSS solution to this bug is likely to be feasible.

(In reply to :Gijs Kruitbosch from comment #20) > As long as all it does is toggle an attribute when that happens, I don't > think that is a problem. Well, it should set the attribute to the state it was already in. If it toggled it, that would actually be a problem. Also, it does more than touching the attribute -- in order to do so, it pokes gBrowser.webNavigation.canGoForward, which isn't free. > I don't think a simple, pure CSS solution to this bug is likely to be feasible. No, but perhaps you could observe the margin transition instead. (That's how we move the button behind the location bar, as far as I remember.)

So I realized that there were some issues with customize mode and with switching between tabs. This patch retains the current behaviour where, if you click forward so that the button becomes disabled, and you then switch tabs while the button is still hovered, the button stays visible until you stop hovering. This is at the expense of checking mozMatchesSelector and getComputedStyle in the method, which I really don't like, but I didn't see a better way to accomplish this, as the transition-delay in our CSS means there's no way to know if/when this has/hasn't happened without checking these things. :-|

Here's an alternative patch, which removes the aforementioned functionality: switching between tabs triggers hiding the button immediately. This means less ugly JS checking, but more complicated CSS. I don't know if hiding it immediately would be considered a bug or a feature, but this code was easier to write. OTOH, not a big fan of making our CSS more complicated. Rock, meet hard place. :-(

Dão, review ping?

Given the review delays, these patches are bitrotten. Gijs: can you update them?

Unbitrotted Gijs' patch and fixed an RTL bug in CombinedBackForward.handleEvent.

Comment on attachment 8342120 [details] [diff] [review] Patch Review of attachment 8342120 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +3857,5 @@ > + handleEvent: function(aEvent) { > + if (aEvent.type == "transitionend" && > + (aEvent.propertyName == "margin-left" || aEvent.propertyName == "margin-right") && > + this.forwardButton.hasAttribute("disabled")) > + this.setForwardButtonOcclusion(true); Nit: Wrap this in {} to make it more readable. @@ +3864,5 @@ > + if (!this.forwardButton) > + return; > + > + if (shouldBeOccluded) > + this.forwardButton.setAttribute("occluded-by-urlbar", "true"); Check if this attribute isn't already set here - since this gets called for every tab-switch, needlessly setting this attribute could be relatively expensive when cycling through several tabs.