Closed Bug 682534 Opened 8 years ago Closed 8 years ago

Implement conditional forward button for winstripe / large icons mode

Categories

(Firefox :: Theme, enhancement)

All
Windows 7
enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 10

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(2 files, 7 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #556244 - Flags: review?(dolske)
Severity: normal → enhancement
Blocks: 677860
Blocks: 674744
Blocks: 682536
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?
(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.
Attached patch patch v2 (obsolete) — Splinter Review
fixed two glitches (RTL spacing, dealing with back/forward being hidden in popups)
Attachment #556244 - Attachment is obsolete: true
Attachment #556244 - Flags: review?(dolske)
Attachment #557167 - Flags: review?(dolske)
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.
Attached patch patch v3 (obsolete) — Splinter Review
(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!
Attachment #557167 - Attachment is obsolete: true
Attachment #557167 - Flags: review?(dolske)
Attachment #557465 - Flags: review?(dolske)
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?
(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.
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).
> 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.
(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.
(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.
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
found a white gap on top-left and bottom-left of verified identity box. http://i.imgur.com/iSCBV.png
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?
(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
(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.
A hidden pref would be the preferred way to disable this. We could easily support this if we had bug 677302.
Attached patch patch v4 (obsolete) — Splinter Review
updated to tip, addressed comment 13
Attachment #557465 - Attachment is obsolete: true
Attachment #557465 - Flags: review?(dolske)
Attachment #560419 - Flags: review?(dolske)
Attached patch patch v5 (obsolete) — Splinter Review
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)
Attachment #560419 - Attachment is obsolete: true
Attachment #560419 - Flags: review?(dolske)
Attachment #560971 - Flags: review?(dolske)
Blocks: 681480
Attached patch patch v5 (obsolete) — Splinter Review
updated to tip
Attachment #560971 - Attachment is obsolete: true
Attachment #560971 - Flags: review?(dolske)
Attachment #562250 - Flags: review?(dolske)
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
Attachment #562250 - Flags: feedback-
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
Attached patch patch v6 (obsolete) — Splinter Review
Attachment #562250 - Attachment is obsolete: true
Attachment #562250 - Flags: review?(dolske)
Attachment #563404 - Flags: review?(dolske)
Attached patch patch v6Splinter Review
missed a selector...
Attachment #563404 - Attachment is obsolete: true
Attachment #563404 - Flags: review?(dolske)
Attachment #563405 - Flags: review?(dolske)
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 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()?
Attachment #563405 - Flags: review?(dolske) → review+
What's the plan for not-winstripe? Will 674744 be landing at the same time, or afterwards?
(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.
(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.
Depends on: 694084
Depends on: 694287
Depends on: 694450
Depends on: 677144
Depends on: 713338
Depends on: 739160
No longer blocks: 700363
Depends on: 700363
You need to log in before you can comment on or make changes to this bug.