682534 - Implement conditional forward button for winstripe / large icons mode

Status: RESOLVED FIXED

(Firefox :: Theme, enhancement)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Jared asked me to help him out with the conditional forward button affecting the horizontal position of other elements in the toolbar. Since I wasn't sure whether what I told him was sane, I implemented this myself. This also handles RTL, which AFAICT was broken with Jared's patch.

Comment 1

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

Comment on attachment 556244 [details] [diff] [review]
patch

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

::: browser/base/content/browser.xul
@@ +490,5 @@
> +                                            this.parentNode.setAttribute('forwarddisabled', 'true');
> +                                          else
> +                                            this.parentNode.removeAttribute('forwarddisabled');">
> +          <observes element="Browser:ForwardOrForwardDuplicate" attribute="disabled"/>
> +        </dummyobservertarget>

In the patch for 674744, I had moved this event handling to browser.js and used the preexisting UpdateBackForwardCommands to notify observers of the changes. While the approach in bug 674744 resulted in more code, I think that it (bug 674744) is a clearer separation of concerns and feels less hackish than adding permanently hidden nodes to the toolbar.

::: browser/themes/winstripe/browser/browser.css
@@ +843,5 @@
>  }
>  
> +@conditionalForwardWithUrlbar@ > #forward-button {
> +  border-top-right-radius: 0;
> +  border-bottom-right-radius: 0;

Should there be a rule for RTL that sets the border-top-left-radius/border-bottom-left-radius to 0?

@@ +1234,5 @@
> +
> +@conditionalForwardWithUrlbar@ + #urlbar-container > #urlbar {
> +  -moz-border-start: none;
> +  margin-left: 0;
> +  -moz-transition: margin-left @forwardTransitionLength@ ease-out;

Similarly, should there be -moz-transition/margin-right rules for RTL?

Comment 2

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jwein] from comment #1)
> In the patch for 674744, I had moved this event handling to browser.js and
> used the preexisting UpdateBackForwardCommands to notify observers of the
> changes. While the approach in bug 674744 resulted in more code, I think
> that it (bug 674744) is a clearer separation of concerns and feels less
> hackish than adding permanently hidden nodes to the toolbar.

I very much dislike the complexity of your code in bug 674744 and don't see a downside of using <observes>.

> > +@conditionalForwardWithUrlbar@ > #forward-button {
> > +  border-top-right-radius: 0;
> > +  border-bottom-right-radius: 0;
> 
> Should there be a rule for RTL that sets the
> border-top-left-radius/border-bottom-left-radius to 0?

No, we mirror the forward button for RTL.

> > +@conditionalForwardWithUrlbar@ + #urlbar-container > #urlbar {
> > +  -moz-border-start: none;
> > +  margin-left: 0;
> > +  -moz-transition: margin-left @forwardTransitionLength@ ease-out;
> 
> Similarly, should there be -moz-transition/margin-right rules for RTL?

No, this patch is using margin-left for both LTR and RTL.

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch v2

fixed two glitches (RTL spacing, dealing with back/forward being hidden in popups)

Comment 4

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

Would it be possible to delay the transition of the forward button until the mouse is no longer hovering over the forward button?

This approach should provide a nice way to prevent users from accidentally clicking on the identity block.

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch v3

(In reply to Jared Wein [:jwein] from comment #4)
> Would it be possible to delay the transition of the forward button until the
> mouse is no longer hovering over the forward button?
> 
> This approach should provide a nice way to prevent users from accidentally
> clicking on the identity block.

done!

Comment 6

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

Comment on attachment 557465 [details] [diff] [review]
patch v3

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

::: browser/themes/winstripe/browser/browser.css
@@ +1263,5 @@
> +}
> +
> +@conditionalForwardWithUrlbar@[forwarddisabled]:not(:hover) + #urlbar-container > #urlbar {
> +  /* when not hovered anymore, trigger a new transition to hide the forward button immediately */
> +  margin-left: -27.01px;

I'm just curious here, what does the extra 0.01px provide?

Comment 7

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jwein] from comment #6)
> > +@conditionalForwardWithUrlbar@[forwarddisabled]:not(:hover) + #urlbar-container > #urlbar {
> > +  /* when not hovered anymore, trigger a new transition to hide the forward button immediately */
> > +  margin-left: -27.01px;
> 
> I'm just curious here, what does the extra 0.01px provide?

It triggers the new transition that the comment is referring to. The delayed transition targets 27px already and repeating this value wouldn't cancel that transition.

Comment 8

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

The patch looks really good, but I wanted to let you know about a few minor issues I have seen:

1. The forward button loses the top/bottom borders when it is disabled. If a user clicks the forward button and doesn't mouse away, then the missing borders are noticeable.

2. Switching from one tab A (where tab A can go forward) to tab B (where tab B cannot go forward) causes an animation of the address bar. This transition seems distracting.

3. The favicon in the address site-identity block should be removed. Sites like Amazon.com that have forward-pointing arrows in their favicon will confuse users with this new setup (not to mention the favicon is also seen in the tab as well).

Comment 9

[Dão Gottwald [:dao]]

> 1. The forward button loses the top/bottom borders when it is disabled. If a
> user clicks the forward button and doesn't mouse away, then the missing
> borders are noticeable.

It doesn't lose the border, it gets a fainter border like all disabled buttons. We can tweak this in a followup bug.

> 2. Switching from one tab A (where tab A can go forward) to tab B (where tab
> B cannot go forward) causes an animation of the address bar. This transition
> seems distracting.

bug 681480 (No reason to track this specifically for each platform, as this can't be fixed solely in the theme.)

> 3. The favicon in the address site-identity block should be removed.

Not in this bug.

Comment 10

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #9)
> > 2. Switching from one tab A (where tab A can go forward) to tab B (where tab
> > B cannot go forward) causes an animation of the address bar. This transition
> > seems distracting.
> 
> bug 681480 (No reason to track this specifically for each platform, as this
> can't be fixed solely in the theme.)

Actually it might be possible to do it solely in the theme at the expense of removing the transition when going back with the keyboard -- which might actually be a good idea. I'll give it a try.

Comment 11

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #10)
> (In reply to Dão Gottwald [:dao] from comment #9)
> > > 2. Switching from one tab A (where tab A can go forward) to tab B (where tab
> > > B cannot go forward) causes an animation of the address bar. This transition
> > > seems distracting.
> > 
> > bug 681480 (No reason to track this specifically for each platform, as this
> > can't be fixed solely in the theme.)
> 
> Actually it might be possible to do it solely in the theme at the expense of
> removing the transition when going back with the keyboard -- which might
> actually be a good idea. I'll give it a try.

No, I think this isn't going to work. It would conflict with the transition being delayed until the mouse leaves the button.

Comment 12

[Ekanan Ketunuti]

I found some problem with this. The notification box (geo, addons, password etc.) doesn't fit with identity box. http://i.imgur.com/pxTv3.png

Comment 13

[Ekanan Ketunuti]

found a white gap on top-left and bottom-left of verified identity box. http://i.imgur.com/iSCBV.png

Comment 14

[solcroft]

Are there any plans to allow disabling the conditional forward button and having it show all the time, yet preserve the default navigation toolbar layout?

Comment 15

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

(In reply to solcroft from comment #14)
> Are there any plans to allow disabling the conditional forward button and
> having it show all the time, yet preserve the default navigation toolbar
> layout?

At this point, I don't think we will provide an explicit option to disable the conditional forward button. However, there is a workaround that should work for you.

1. Right-click on the toolbar and choose Customize...
2. Drag a Flexible Space between the forward button and the URL bar.
3. Click Done

Comment 16

[solcroft]

(In reply to Jared Wein [:jwein] from comment #15)
> At this point, I don't think we will provide an explicit option to disable
> the conditional forward button. However, there is a workaround that should
> work for you.

Hi Jared,

Appreciate the reply, but I'm looking for a solution that preserves the default navigation toolbar layout.

It doesn't have to be a setting that can be toggled in about:config, I'm perfectly happy with resorting to aa css solution for now. Does any such solution exist, short of hacking the omni.jar file?

Thanks in advance.

Comment 17

[Dão Gottwald [:dao]]

A hidden pref would be the preferred way to disable this. We could easily support this if we had bug 677302.

Comment 18

[Dão Gottwald [:dao]]

Attachment: patch v4

updated to tip, addressed comment 13

Comment 19

[Dão Gottwald [:dao]]

Attachment: patch v5

used the time waiting for the review to fix a minor glitch Asa and Jared told me about as well as bug 681480 (for winstripe)

Comment 20

[Dão Gottwald [:dao]]

Attachment: patch v5

updated to tip

Comment 21

[Frank Yan (:fryn)]

Comment on attachment 562250 [details] [diff] [review]
patch v5

This breaks <textbox newlines="stripsurroundingwhitespace"/>.

See the test failures in mochitest-other here:
https://tbpl.mozilla.org/?tree=UX&rev=a577d1958afb

Log excerpt:
chrome://mochitests/content/browser/browser/base/content/test/browser_bug321000.js
| Urlbar strips newlines and surrounding whitespace
- Got   hello hello      world world  , expected   hello helloworldworld

MXR link:
https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_bug321000.js#41

I have also reproduced this by manually copying and pasting multi-line selections.

I debugged this, and it looks like setting the |switchingtabs| attribute to true on the root element at any time triggers this, even if the attribute is later removed. It seems like whatever CSS is being applied while the attribute exists causes that stuff to break permanently.

Also, |-moz-transition: 0 padding-left;| doesn't work.
Even though the current spec draft specifies 0 as a valid transition duration and also the default value, 0 is invalid in our current implementation:
http://www.w3.org/TR/css3-transitions/#the-transition-duration-property-
https://developer.mozilla.org/en/CSS/transition-duration

Comment 22

[Frank Yan (:fryn)]

This is currently on the UX branch and causing test failures, but it's rather heavy-handed to back it out just for that failure, so I fixed the failure with the temporary workaround of toggling gURLBar.style.MozTransition instead of the switchingtabs attribute. I also replaced the transition-durations 0 with 0s.
https://hg.mozilla.org/projects/ux/rev/16ae14b8c628

Comment 23

[Dão Gottwald [:dao]]

Attachment: patch v6

Comment 24

[Dão Gottwald [:dao]]

Attachment: patch v6

missed a selector...

Comment 25

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

Attachment: Forward button bleeding through on hover of back button

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.

Comment 26

[Justin Dolske [:Dolske]]

Comment on attachment 563405 [details] [diff] [review]
patch v6

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

Boy do I hate CSS sometimes. :)

::: browser/base/content/browser.xul
@@ +1010,5 @@
>        <svg:rect x="0" y="0" width="1" height="1" fill="white"/>
>        <svg:circle cx="-0.35" cy="0.5" r="0.58"/>
>      </svg:mask>
> +    <svg:mask id="winstripe-urlbar-back-button-mask" maskContentUnits="userSpaceOnUse">
> +      <svg:rect x="0" y="0" width="1000000" height="50" fill="white"/>

What's the memory impact of this?

Specifically, I don't know enough about SVG to know for sure that something won't create some transient 50 megapixel (200MB!) thing, which would obviously not be good. (I don't see any sign of that happening in about:memory, though.)

::: browser/themes/winstripe/browser/browser.css
@@ +1229,5 @@
>  }
>  
> +@conditionalForwardWithUrlbar@ + #urlbar-container {
> +  padding-left: 27px;
> +  -moz-margin-start: -27px;

The "27" value is used in a few places, where does this come from? Worth a comment, @define, or calc()?

Comment 27

[Justin Dolske [:Dolske]]

What's the plan for not-winstripe? Will 674744 be landing at the same time, or afterwards?

Comment 28

[Dão Gottwald [:dao]]

(In reply to Justin Dolske [:Dolske] from comment #26)
> What's the memory impact of this?
> 
> Specifically, I don't know enough about SVG to know for sure that something
> won't create some transient 50 megapixel (200MB!) thing, which would
> obviously not be good. (I don't see any sign of that happening in
> about:memory, though.)

I don't think it should have a memory impact. I can reduce the width, though.

> > +@conditionalForwardWithUrlbar@ + #urlbar-container {
> > +  padding-left: 27px;
> > +  -moz-margin-start: -27px;
> 
> The "27" value is used in a few places, where does this come from? Worth a
> comment, @define, or calc()?

This just happens to be the width of the forward button. I can use a @define.

(In reply to Justin Dolske [:Dolske] from comment #27)
> What's the plan for not-winstripe? Will 674744 be landing at the same time,
> or afterwards?

It will land afterwards.

Comment 29

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e1927273c6c9

Comment 30

[Dão Gottwald [:dao]]

typo fix: http://hg.mozilla.org/integration/mozilla-inbound/rev/fd292a050695

Comment 31

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/e1927273c6c9
https://hg.mozilla.org/mozilla-central/rev/fd292a050695

Comment 32

[u420019]

(In reply to Jared Wein [:jwein] from comment #15)
> (In reply to solcroft from comment #14)
> > Are there any plans to allow disabling the conditional forward button and
> > having it show all the time, yet preserve the default navigation toolbar
> > layout?
> 
> At this point, I don't think we will provide an explicit option to disable
> the conditional forward button. However, there is a workaround that should
> work for you.
> 
> 1. Right-click on the toolbar and choose Customize...
> 2. Drag a Flexible Space between the forward button and the URL bar.
> 3. Click Done

I don't like the conditional forward button too and I know that solution already. But is there a way to do this without the gap between the arrows and the address bar? Maybe with CSS?

IMO, a setting to turn on and off conditional forward button would be great.