Implement conditional forward button for winstripe / large icons mode

RESOLVED FIXED in Firefox 10

Status

()

Firefox
Theme
--
enhancement
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

Trunk
Firefox 10
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 556244 [details] [diff] [review]
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.
Attachment #556244 - Flags: review?(dolske)
(Assignee)

Updated

6 years ago
Severity: normal → enhancement
(Assignee)

Updated

6 years ago
Blocks: 677860
(Assignee)

Updated

6 years ago
Blocks: 674744
(Assignee)

Updated

6 years ago
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?
(Assignee)

Comment 2

6 years ago
(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.
(Assignee)

Comment 3

6 years ago
Created attachment 557167 [details] [diff] [review]
patch v2

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.
(Assignee)

Comment 5

6 years ago
Created attachment 557465 [details] [diff] [review]
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!
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?
(Assignee)

Comment 7

6 years ago
(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).
(Assignee)

Comment 9

6 years ago
> 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.
(Assignee)

Comment 10

6 years ago
(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.
(Assignee)

Comment 11

6 years ago
(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

6 years ago
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

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

Comment 14

6 years ago
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

Comment 16

6 years ago
(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.
(Assignee)

Comment 17

6 years ago
A hidden pref would be the preferred way to disable this. We could easily support this if we had bug 677302.
(Assignee)

Comment 18

6 years ago
Created attachment 560419 [details] [diff] [review]
patch v4

updated to tip, addressed comment 13
Attachment #557465 - Attachment is obsolete: true
Attachment #557465 - Flags: review?(dolske)
Attachment #560419 - Flags: review?(dolske)
(Assignee)

Comment 19

6 years ago
Created attachment 560971 [details] [diff] [review]
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)
Attachment #560419 - Attachment is obsolete: true
Attachment #560419 - Flags: review?(dolske)
Attachment #560971 - Flags: review?(dolske)
(Assignee)

Updated

6 years ago
Blocks: 681480
(Assignee)

Comment 20

6 years ago
Created attachment 562250 [details] [diff] [review]
patch v5

updated to tip
Attachment #560971 - Attachment is obsolete: true
Attachment #560971 - Flags: review?(dolske)
Attachment #562250 - Flags: review?(dolske)

Comment 21

6 years ago
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-

Comment 22

6 years ago
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
(Assignee)

Comment 23

6 years ago
Created attachment 563404 [details] [diff] [review]
patch v6
Attachment #562250 - Attachment is obsolete: true
Attachment #562250 - Flags: review?(dolske)
Attachment #563404 - Flags: review?(dolske)
(Assignee)

Comment 24

6 years ago
Created attachment 563405 [details] [diff] [review]
patch v6

missed a selector...
Attachment #563404 - Attachment is obsolete: true
Attachment #563404 - Flags: review?(dolske)
Attachment #563405 - Flags: review?(dolske)
Created attachment 565040 [details]
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 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?
(Assignee)

Comment 28

6 years ago
(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.
(Assignee)

Comment 29

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/e1927273c6c9
Target Milestone: --- → Firefox 10
(Assignee)

Comment 30

6 years ago
typo fix: http://hg.mozilla.org/integration/mozilla-inbound/rev/fd292a050695
https://hg.mozilla.org/mozilla-central/rev/e1927273c6c9
https://hg.mozilla.org/mozilla-central/rev/fd292a050695
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 32

6 years ago
(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.
(Assignee)

Updated

6 years ago
Depends on: 694084
Depends on: 694287
(Assignee)

Updated

6 years ago
Depends on: 694450
Depends on: 699257
Blocks: 700363

Updated

6 years ago
Depends on: 677144

Updated

5 years ago
Depends on: 713338
(Assignee)

Updated

5 years ago
Depends on: 739160
(Assignee)

Updated

5 years ago
No longer blocks: 700363
Depends on: 700363
You need to log in before you can comment on or make changes to this bug.