Closed Bug 700363 Opened 8 years ago Closed 6 years ago

Forward button bleeding through on hover of back button (Conditional forward button)

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: jaws, Assigned: Gijs)

References

Details

Attachments

(2 files, 7 obsolete files)

Attached image Screenshot of bug
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]
Duplicate of this bug: 724206
Blocks: 674744, 682534
No longer depends on: 674744, 682534
Duplicate of this bug: 747686
Assignee: nobody → soapyhamhocks
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #617322 - Flags: review?(dao)
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
Attachment #617322 - Flags: review?(dao) → review-
Duplicate of this bug: 750127
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.
Duplicate of this bug: 893367
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).
Attachment #811393 - Flags: review?(jaws)
Assignee: soapyhamhocks → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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
Attachment #811393 - Flags: review?(jaws) → review-
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.
Attachment #811393 - Flags: review- → review?(jaws)
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.
Attachment #811393 - Flags: review?(jaws) → review-
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.
Attachment #811484 - Flags: review?(jaws)
Attachment #811393 - Attachment is obsolete: true
For reference, this is what the patch looks like when merged on UX
Attachment #811484 - Attachment is obsolete: true
Attachment #811484 - Flags: review?(jaws)
Attachment #811484 - Attachment is obsolete: false
Attachment #811484 - Flags: review?(jaws)
Attachment #811485 - Attachment filename: 700363-conditional-fwd-button-glitch → 700363-ux-conditional-fwd-button-glitch
I forgot that the style bits should only apply when the conditional forward button is in use, oops.
Attachment #811487 - Flags: review?(jaws)
Attachment #811484 - Attachment is obsolete: true
Attachment #811484 - Flags: review?(jaws)
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.
Attachment #811487 - Flags: review?(jaws) → review-
(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. :-|
Attachment #811917 - Flags: review?(dao)
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. :-(
Attachment #811918 - Flags: review?(dao)
Attachment #811487 - Attachment is obsolete: true
Attachment #811918 - Attachment description: hide conditional forward button permanently when not in use to prevent accidental glitches, → hide conditional forward button permanently when not in use (also hide immediately on tabswitch)
Attachment #811485 - Attachment is obsolete: true
Duplicate of this bug: 940762
Dão, review ping?
Given the review delays, these patches are bitrotten. Gijs: can you update them?
Attached patch PatchSplinter Review
Unbitrotted Gijs' patch and fixed an RTL bug in CombinedBackForward.handleEvent.
Attachment #617322 - Attachment is obsolete: true
Attachment #811917 - Attachment is obsolete: true
Attachment #811918 - Attachment is obsolete: true
Attachment #811917 - Flags: review?(dao)
Attachment #811918 - Flags: review?(dao)
Attachment #8342120 - Flags: review?(dao)
Attachment #8342120 - Flags: review?(bmcbride)
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.
Attachment #8342120 - Flags: review?(dao)
Attachment #8342120 - Flags: review?(bmcbride)
Attachment #8342120 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b0a6b666b291
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.