674744 - Implement conditional forward button for pinstripe

Status: RESOLVED FIXED

(Firefox :: Theme, enhancement)

Description

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

The Back button should be connected to the URL bar. The Forward button will be visually placed within the URL bar and will slide under the Back button when there is no forward navigation available.

Comment 1

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

Attachment: WIP for bug 674744

This is the current WIP of a patch for this bug. This WIP is only for the Winstripe theme with mode=icons and iconsize=large

Comment 2

[Dão Gottwald [:dao]]

Technical note: It seems that this should be doable with the back/forward buttons kept as the location bar's siblings, rather than children of the location bar.

Comment 3

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

(In reply to comment #2)
> Technical note: It seems that this should be doable with the back/forward
> buttons kept as the location bar's siblings, rather than children of the
> location bar.

It would be great to keep them as siblings, as it would solve many of the issues that we would encounter.

I could probably use positioning to place the unified-back-forward button on top of the address bar, but then animating the forward button won't trigger the URL to move. Do you have any recommendations as to how to keep them siblings and continue to make the URL animate with the forward button?

Comment 4

[Dão Gottwald [:dao]]

It sounds like you want a second parallel transition that changes the urlbar's left padding or something like that.

Comment 5

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

(In reply to comment #4)
> It sounds like you want a second parallel transition that changes the
> urlbar's left padding or something like that.

Yes. I'm interested if you had a clever way to do this?

I'm currently using the disabled attribute to transition the forward button, but the urlbar currently doesn't receive an attribute like this. I can look in to adding one, but I wasn't sure I'm not sure if that is the wrong direction.

Comment 6

[Dão Gottwald [:dao]]

There could be a "forwarddisabled" attribute on unified-back-forward-button (the forward button's parent node), and then you'd use #unified-back-forward-button[forwarddisabled] + #urlbar-container > #urlbar.

Comment 7

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

(In reply to comment #6)
> There could be a "forwarddisabled" attribute on unified-back-forward-button
> (the forward button's parent node), and then you'd use
> #unified-back-forward-button[forwarddisabled] + #urlbar-container > #urlbar.

Great idea! Thanks :)

Comment 8

[bogas04]

Considering these mockups http://people.mozilla.com/~shorlander/ux-presentation/ux-presentation.html , and the OneLiner addon which Mozilla Labs published this week , i believe this should be implemented for trial in UX builds.

Comment 9

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

Attachment: WIP for bug 674744

This is a basic implementation of the conditional forward button. There are some rough edges but all functionality should be present.

Known issues are:
- Jankiness when hovering over the back button when the forward button is missing.
- Customize dialog looks janky when buttons are added between the unified back forward button, but the jankiness will go away when the Customize dialog is closed.
- Lack of support for Linux.
- Lightweight themes (personas) on Mac cause the url bar to show through the Back button.

Comment 10

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

Attachment: WIP for bug 674744 v3

Fixed the jankiness when hovering over the back button if the forward button is disabled.

Comment 11

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

Attachment: WIP for bug 674744 v4

Known issues at this point:
- Lack of "back-button glow" on hover.
- Opening the Customize dialog can cause the disabled look-and-feel of the back/forward button to get out of sync.

Comment 12

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

The basic functionality and look-and-feel of the conditional forward button has been implemented and can be seen in UX builds.

Here is the list of known issues at this point:
- If the bookmarks button is placed between the back/forward button and the urlbar, with the addition of the bookmarks toolbar enabled, then the forward button will not be conditional. A follow-up bug will need to be filed for this case.
- Lack of support for Linux. Linux support will have to wait until we get the theme improvements for gnomestripe to add in a rounded back button, etc.
- The back button does not have the background-image glow like other toolbar buttons. This is because I have made the back button fully opaque. Additionally, the back button may look slightly different than other toolbar buttons due to its opaqueness.
- The patch-so-far uses a graphic that I made, Toolbar-disabled.png, which includes a faded-out large back arrow. This is the only element of the graphic that is being used currently, and it is because we cannot use transparency on the back button.
- Opening the Customize dialog can cause the disabled look-and-feel of the back/forward button to get out of sync. Opening a new tab or restarting the browser seems to be the only way to fix this issue at the moment.
- The site identity block has been removed for the time being. This is not a permanent change, as it has been removed until we have a good idea of how we want to display site identity information.

Comment 13

[bogas04]

Is it going to be implemented for small icons mode too?

Comment 14

[Girish Sharma [:Optimizer]]

If this is the bug being pushed into UX branch for about three times in 3 days then I have an issue : 
When any button is before the unified forward back button , and urlbar is next to unified back forward button , then do the following : 
1. have a page in history to go forward to , but nothing to go back to 
2. press forward and forward button hides . 
3. press back 
4. press forward now and he urlbar go behind the back button far left to the screen edge

Comment 15

[Jake]

(In reply to scrapmachines from comment #14)

Confirmed, I can reproduce that using the provided steps. Adding or removing an element behind the back button makes the URL bar move a for a "spot" too far to the left. Restarting the browser seems to fix it though.

Comment 16

[Edward Forreal]

I second the question about small icons from comment 13.
Any plans on that, guys?

Comment 17

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

(In reply to bogas04 from comment #13)
> Is it going to be implemented for small icons mode too?

(In reply to Edward Forreal from comment #16)
> I second the question about small icons from comment 13.
> Any plans on that, guys?

I'm sorry, but as far as I understand, this will only be implemented for large icon mode.

If you would like this to be implemented for small icon mode, please file a bug to request it for small icons.

Comment 18

[Edward Forreal]

(In reply to Jared Wein [:jwein] from comment #17)

Thanks Jared. Filed Bug 677860.

Comment 19

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

Attachment: WIP for bug 674744 v5

Pushed to UX branch:
https://hg.mozilla.org/projects/ux/rev/83c065b68324

Pushed to try server:
http://tbpl.mozilla.org/?tree=Try&rev=384ae169ea1d

Comment 20

[Theodore]

Attachment: save password prompt covers url

I noticed a bug when the save password prompt appears: the "arrow icon" for it partially covers the beginning of the url in the location bar.

Comment 21

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

(In reply to Theodore from comment #20)
> Created attachment 552664 [details]
> save password prompt covers url
> 
> I noticed a bug when the save password prompt appears: the "arrow icon" for
> it partially covers the beginning of the url in the location bar.

Thank you for letting me know about this. I will see what I can do to fix that.

Comment 22

[Darren Kalck [:D-Kalck]]

When you set small icons and set big icons back the forward button is always shown.

Comment 23

[Wesley Johnston (:wesj)]

Not sure if its known, but this has problems with personas on osx:

http://dl.dropbox.com/u/72157/screeny.png

Comment 24

[bogas04]

popup windows lacking back button have shifted location bar problem

Comment 25

[Theodore]

Attachment: Screenshot of shifted location bar in popups problem

Screenshot of the problem pointed out in comment 24.

Comment 26

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

(In reply to Darren Kalck [:D-Kalck] from comment #22)
(In reply to Wesley Johnston (:wesj) from comment #23)
(In reply to bogas04 from comment #24)
(In reply to Theodore from comment #25)

Thank you for the great feedback. I have noted these issues and they should be fixed in a new UX branch build within a couple days.

Comment 27

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

Attachment: WIP for bug 674744 v6

fryn: Do you have any tips to get around my "broadcasteventhax"? I'm trying to sync the forward navigation disabled event to other elements, but I want to rename the attribute in the process.

Also, any other general feedback you may have would be greatly appreciated :)

Comment 28

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

Attachment: WIP for bug 674744 v7

Comment 29

[Siddhartha Dugar [:sdrocking]]

Attachment: Back button overlaps urlbar suggestions

Comment 30

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

Attachment: WIP for bug 674744 v8

fryn: I have removed the previous hack from the older patch. Can you please give me feedback?

Comment 31

[Frank Yan (:fryn)]

Attachment: WIP v8 on Snow Leopard

Jared, here's a screen recording of WIP v8 on Snow Leopard.
Note the asymmetric identity box, the distorted (lengthened) forward button, the jitter of the animation, and the slight difference in toolbar item widths between the before/after states.

The patch also regresses startup performance by causing the UI to have initial ephemeral glitches and to require several reflows after the window is painted to settle in its final state. Like the combined reload/stop/go in the url bar, the conditional forward button should be in its correct state (for default configurations) by the time the window is first painted.

I'll provide code feedback shortly.

Comment 32

[Siddhartha Dugar [:sdrocking]]

The awesome bar suggestions start from below the Back button, which a deviation from the mockups (https://wiki.mozilla.org/images/a/a8/LocationBar-Results-i01.jpg). Please fix this.

Comment 33

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

(In reply to sdrocking from comment #29)
> Created attachment 554797 [details]
> Back button overlaps urlbar suggestions

(In reply to sdrocking from comment #32)
> The awesome bar suggestions start from below the Back button, which a
> deviation from the mockups
> (https://wiki.mozilla.org/images/a/a8/LocationBar-Results-i01.jpg). Please
> fix this.

Thanks sdrocking for letting me know about this. I have it on my TODO list to fix it :)

Comment 34

[Girish Sharma [:Optimizer]]

Also , in the latest UX builds (hourly) . I noticed a glitch .
When Home button is between unified back/forward button and urlbar, Clicking on Cusomize or toolbar layout (ie trying to change the position of unified back/forward button ) fails and nothing happes, except the disabling of customize and toolbar layout option.
Customize/Toolbar Layout works if unified back/forward button is already just before urlbar.

Comment 35

[Ekanan Ketunuti]

Since UX cset 649d951c0c86. I'm unable to customize toolbar if 'Use Small Icons' enabled.

STR
1. start with clean profile.
2. customize toolbar, checked 'Use small icons'.
3. customize toolbar again

the customize toolbar window doesn't appear and i get following error.

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://browser/content/browser.js :: uninit :: line 10555"  data: no]

I cannot reproduce this on cset 60b0b3ad4dd3.

Comment 36

[bogas04]

(In reply to scrapmachines from comment #34)
> Also , in the latest UX builds (hourly) . I noticed a glitch .
> When Home button is between unified back/forward button and urlbar, Clicking
> on Cusomize or toolbar layout (ie trying to change the position of unified
> back/forward button ) fails and nothing happes, except the disabling of
> customize and toolbar layout option.
> Customize/Toolbar Layout works if unified back/forward button is already
> just before urlbar.

i second that

Comment 37

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

(In reply to scrapmachines from comment #34)
(In reply to Ek from comment #35)
(In reply to bogas04 from comment #36)

Thank you for your detailed steps to reproduce scrapmachines and Ek. I believe I have fixed this locally and will try to push out a new patch to the UX build tomorrow.

Comment 38

[Dão Gottwald [:dao]]

Winstripe spun off to bug 682534. The patch there adds a forwarddisabled attribute on unified-back-forward-button, which should be used here.

Comment 39

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

Attachment: WIP of patch for bug 674744

This is an updated version of the patch that only applies to pinstripe. These are the known issues:
1. The toolbar items shift when the forward button appears/disappears.
2. The notification popup has not been tested with this version.
3. The border-radius on the forward button needs to be removed.

Comment 40

[Frank Yan (:fryn)]

Jared, while this no longer applies to the current patch, for future reference, I think `toolbar.mode` can be used instead of `toolbar.getAttribute("mode")`. https://developer.mozilla.org/en/XUL/Attribute/toolbar.mode

Comment 41

[Dão Gottwald [:dao]]

(In reply to Frank Yan [:fryn] from comment #40)
> Jared, while this no longer applies to the current patch, for future
> reference, I think `toolbar.mode` can be used instead of
> `toolbar.getAttribute("mode")`.
> https://developer.mozilla.org/en/XUL/Attribute/toolbar.mode

There's no "mode" property.

Comment 42

[Frank Yan (:fryn)]

(In reply to Dão Gottwald [:dao] from comment #41)
> > https://developer.mozilla.org/en/XUL/Attribute/toolbar.mode
> There's no "mode" property.

Indeed there isn't. I was mistaken. What a confusing title. The path implies that it's an attribute, but the article title implies otherwise. Likewise for <https://developer.mozilla.org/en/XUL/Attribute/resizer.dir>. There ought to be a better notation...

Comment 43

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

Attachment: WIP of patch for bug 674744 i2

Currently still needs better support for styling the themes for Leopard and Snow Leopard. There is also a small glitch with the padding of the identity box when the forward button becomes disabled (before it disappears).

Comment 44

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

Attachment: Patch for bug 674744

This has been pushed to the UX branch:
https://hg.mozilla.org/projects/ux/rev/32341b414d17

Comment 45

[Nguyen Bat Hung]

Attachment: When the forward button is actived, identity box shouldn't have boder radius (when coditional forward button enble)

Have forward button active
Click on the identity box
Expected: top-left and bottom-left corners of identity box should be square, fit the location bar connected to forward button
In fact: the identity box top-left and bottom-left corners is round.

Comment 46

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

Attachment: Patch for bug 674744

Stephen: Can you please send me the correct border colors for Snow Leopard and also test it out on Lion?

Dao: I have tried tweaking the values for the amount that the #urlbar should move, but I can't find a value that won't cause the other toolbaritems to shift. Can you help me out here?

Comment 47

[bogas04]

Attachment: Addon Installation notification is messed up

Comment 48

[Dão Gottwald [:dao]]

Comment on attachment 561643 [details]
Addon Installation notification is messed up

This is from bug 684450.

Comment 49

[bogas04]

ThankYou and Apologies

Comment 50

[Dão Gottwald [:dao]]

> Dao: I have tried tweaking the values for the amount that the #urlbar should
> move, but I can't find a value that won't cause the other toolbaritems to
> shift. Can you help me out here?

I'm not sure what you mean by "cause the other toolbaritems to shift." Wrong values should only cause the url bar to overlap the back button wrongly.

Comment 51

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

(In reply to Dão Gottwald [:dao] from comment #50)
> > Dao: I have tried tweaking the values for the amount that the #urlbar should
> > move, but I can't find a value that won't cause the other toolbaritems to
> > shift. Can you help me out here?
> 
> I'm not sure what you mean by "cause the other toolbaritems to shift." Wrong
> values should only cause the url bar to overlap the back button wrongly.

|margin-left: -28px;| and |padding-left: 28px| on the urlbar-container fits the correct amount of space to draw the forward button without overlapping the back button.

However, if we set |margin-left: -28px;| on the urlbar, then the width of the urlbar-container seems to shrink a little and the search box moves over.

Comment 52

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

Attachment: Patch for bug 674744 v13

Unbitrotted and fixed two typos.

Comment 53

[Stephen Horlander [:shorlander]]

Attachment: Screenshot of Patch v13

Something not right with the latest patch. Or my build is messed up :)

Comment 54

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

Attachment: Patch for bug 674744 v13 (rebased)

I'm not sure why there is a gap in the screenshot that was attached (https://bugzilla.mozilla.org/attachment.cgi?id=564188).

It looks like the forward navigation was possible but the opacity of the forward button wasn't updated to show the button? Could this be coming from bug 691218? Also, did you apply the patch for bug 682534 before applying this patch (674744 is dependent on 682534)?

Comment 55

[Stephen Horlander [:shorlander]]

(In reply to Jared Wein [:jwein] from comment #54)
> Also, did you apply the patch for bug 682534 before applying
> this patch (674744 is dependent on 682534)?

I did not. Thanks!

Comment 56

[Stephen Horlander [:shorlander]]

Comment on attachment 564225 [details] [diff] [review]
Patch for bug 674744 v13 (rebased)

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

A few things I noticed:
- The mask is not properly sized(aligned?) so the background is bleeding through the back-button
- Hovering over the back-button causes the forward button to reappear behind the location-bar
- The forward-button has a dark drop-shadow, it should match the white etch from the location-bar
- The animation still causes the location-bar+search-field to shift on a new profile unless you manually resize it first

I will attach a screenshot.

Comment 57

[Stephen Horlander [:shorlander]]

Attachment: Screenshot of Patch v13

Comment 58

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

Attachment: Patch for bug 674744 v14

This patch fixes the mask issues and also works now with Personas.

There are two issues still with the shifting of the toolbar items and the forward button bleeding through.

The "forward button bleeding through" issue can also be seen in bug 682534.

Comment 59

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

Attachment: Patch for bug 674744 v15

Updated the patch to use a constant for the width of the forward button.

Comment 60

[Dão Gottwald [:dao]]

Comment on attachment 566405 [details] [diff] [review]
Patch for bug 674744 v15

>+@conditionalForwardWithUrlbar@ + #urlbar-container > #urlbar {
>+  position: relative;

Is this needed? The winstripe patch didn't have this.

Half of this patch's file size is from toolbar-noise-lion.png, which seems unexpectedly large. Can this file be optimized? Could you get rid of it entirely by making the button texture transparent?

Comment 61

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

Attachment: Patch for bug 674744 v16

The |position: relative;| was unneeded.

We could probably make the texture smaller so the file size isn't as large.I'm not sure how we would want to change the color values of the linear gradients if we made the back button transparent.

Comment 62

[Stephen Horlander [:shorlander]]

(In reply to Jared Wein [:jwein] from comment #61)
> Created attachment 566633 [details] [diff] [review] [diff] [details] [review]
> Patch for bug 674744 v16
> 
> The |position: relative;| was unneeded.
> 
> We could probably make the texture smaller so the file size isn't as
> large.I'm not sure how we would want to change the color values of the
> linear gradients if we made the back button transparent.

I have styles that would work to make it translucent. Does it ever overlap anything that isn't the toolbar background?

Comment 63

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

(In reply to Stephen Horlander from comment #62)
> (In reply to Jared Wein [:jwein] from comment #61)
> > Created attachment 566633 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > Patch for bug 674744 v16
> > 
> > The |position: relative;| was unneeded.
> > 
> > We could probably make the texture smaller so the file size isn't as
> > large.I'm not sure how we would want to change the color values of the
> > linear gradients if we made the back button transparent.
> 
> I have styles that would work to make it translucent. Does it ever overlap
> anything that isn't the toolbar background?

Not that I can think of.

Comment 64

[Alex Limi (:limi) — Firefox UX Team]

(In reply to Dão Gottwald [:dao] from comment #60)
> Half of this patch's file size is from toolbar-noise-lion.png, which seems
> unexpectedly large. Can this file be optimized? Could you get rid of it
> entirely by making the button texture transparent?

Jared, if this PNG is coming from Photoshop, just run optipng on it. Example instructions to remove unnecessary layers + alpha:

Remove gamma and color profiles:
  pngcrush -rem gAMA -rem cHRM -rem iCCP -rem sRGB

Optimize file size:
  optipng -o7

Comment 65

[Dão Gottwald [:dao]]

Comment on attachment 566633 [details] [diff] [review]
Patch for bug 674744 v16

>+      <svg:rect x="0" y="-5" width="1000000" height="55" fill="white"/>

nit: use width="10000" like I did for winstripe.

Cancelling review request as per previous comments.

Comment 66

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

Attachment: Patch for bug 674744 v17

Running pngcrush and optipng shrunk the file size by 22 bytes (probably not as much as hoped). I have also fixed the width to match the winstripe implementation.

Comment 67

[Dão Gottwald [:dao]]

Comment on attachment 567795 [details] [diff] [review]
Patch for bug 674744 v17

As I understand it, we don't need the image at all.

Comment 68

[Stephen Horlander [:shorlander]]

(In reply to Dão Gottwald [:dao] from comment #67)
> Comment on attachment 567795 [details] [diff] [review] [diff] [details] [review]
> Patch for bug 674744 v17
> 
> As I understand it, we don't need the image at all.

Yeah I think we can probably get away with just using translucent styles if it doesn't overlap anything else.

I will try it and see.

Comment 69

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

Attachment: Patch for bug 674744 v18

I have updated the CSS to use new styles provided by shorlander that remove the need for the toolbar-noise-lion.png.

Comment 70

[Dão Gottwald [:dao]]

Comment on attachment 572168 [details] [diff] [review]
Patch for bug 674744 v18

>+@conditionalForwardWithUrlbar@ > #forward-button:not(:-moz-lwtheme) {
>+  -moz-appearance: none;
>+  -moz-padding-start: 2px;
>+  background: -moz-linear-gradient(hsl(0,0%,99%), hsl(0,0%,67%));
>+  background-clip: padding-box;

nit:
background: -moz-linear-gradient(hsl(0,0%,99%), hsl(0,0%,67%)) padding-box;

>+@conditionalForwardWithUrlbar@ > #forward-button:hover:active:not(:-moz-lwtheme) {
>+  background: -moz-linear-gradient(hsl(0,0%,74%), hsl(0,0%,61%));
>+  background-clip: padding-box;

background-image: -moz-linear-gradient(hsl(0,0%,74%), hsl(0,0%,61%));

>+@conditionalForwardWithUrlbar@ > #forward-button:-moz-window-inactive:not(:-moz-lwtheme) {
>+  border-color: hsl(0,0%,64%) hsl(0,0%,65%) hsl(0,0%,66%);
>+  background: -moz-linear-gradient(hsl(0,0%,99%), hsl(0,0%,82%));

Are you sure you want to reset background-clip here?

>+@conditionalForwardWithUrlbar@ #identity-box:-moz-locale-dir(ltr),
>+@conditionalForwardWithUrlbar@ #identity-box:-moz-locale-dir(rtl) {
>+  border-radius: 0;
>+}

This selector won't match the identity box.

Comment 71

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

Attachment: Patch for bug 674744 v18.1

Thanks for the quick review! I've addressed your review comments in this new version.

Comment 72

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

There is an issue with the (hidden) forward button bleeding through on hover of the back button. I've filed bug 700363 as a follow-up bug.

Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/e27e65738126

Comment 73

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/e27e65738126

Comment 74

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Is it expected that this behavior is only present in fresh profiles but not existing ones?

Comment 75

[Reuben Morais [:reuben]]

Not sure if this should be a follow up bug, but can the conditional forward button be implemented in a way that adapts to the urlbar height?

That would be more future proof and would also fix the problem with add-ons that put icons in the urlbar, like Read It Later and FlagFox. Some of those icons increase the urlbar height, which in turn makes the conditional forward button look ugly: http://cl.ly/CT1s

Comment 76

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) from comment #74)
> Is it expected that this behavior is only present in fresh profiles but not
> existing ones?

This has been caused by the home button which was located between the back/forward buttons and the location bar. Moving it to the default location at the end of the navbar fixes it. I still wonder how many people will not be able to see this new feature because they are using older profiles with the home button left of the location bar.

Comment 77

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

(In reply to Reuben Morais [:reuben] from comment #75)
> Not sure if this should be a follow up bug, but can the conditional forward
> button be implemented in a way that adapts to the urlbar height?
> 
> That would be more future proof and would also fix the problem with add-ons
> that put icons in the urlbar, like Read It Later and FlagFox. Some of those
> icons increase the urlbar height, which in turn makes the conditional
> forward button look ugly: http://cl.ly/CT1s

Yeah, that looks really ugly. Although I think the fix for that is forcing the urlbar height, because if the forward button grows in size it will look equally ugly. Can you please see if a bug has been filed for this or file a bug if not?

Comment 78

[Dão Gottwald [:dao]]

This is a bug in the Read It Later and FlagFox add-ons, not in our code.

Comment 79

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

(In reply to Dão Gottwald [:dao] from comment #78)
> This is a bug in the Read It Later and FlagFox add-ons, not in our code.

Yeah I agree, but I think there is a bug in our code such that we allow an add-on to inadvertently increase the height of the urlbar. Most users will not associate an add-on with the reason the urlbar + forward button look ugly.

Comment 80

[Reuben Morais [:reuben]]

(In reply to Dão Gottwald [:dao] from comment #78)
> This is a bug in the Read It Later and FlagFox add-ons, not in our code.

All they do is insert a 16x16px image in the urlbar with a XUL overlay, and the problem only happens after the bookmark icon is shown. I've filed bug 709435 with more details.