974229 - Remove obsolete star-button element

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Dave Garrett]

Inside 'urlbar', in 'urlbar-icons', there are two elements IDed: 'page-report-button' & 'star-button'. The later is just a shim left in for the old star button that was in the address bar, presumably for compatibility reasons. It has no image set for it anymore, however it does still take up space and affect layout. It has 3px padding to each side, so this vestigial element seems to add 6px of blank space here. This element even still hides when the URL is edited, shrinking the blank space here in that instance.

If there are other icons shown in 'urlbar-icons', such as the page report button or an icon from an addon (e.g. Flagfox), then this will mess up the icon positioning.

This element should be fully hidden, not just lack an image. Most general fix would be something like:
image[src=""].urlbar-icon { display: none; }

The simplest fix would just be to stick a 'hidden="true"' onto the 'star-button' shim element.

The 'page-report-button' element is fully hidden.

Comment 1

[Dave Garrett]

https://hg.mozilla.org/mozilla-central/file/a66eb5385fde/browser/base/content/browser.xul#l709

The page report button has a 'hidden="true"', so doing that for the star button too would make the most sense.

There's also an 'onclick' attribute left here that probably doesn't need to be there anymore.

Just removing it completely would also be fine of course, but I don't know if it is still needed for compatibility or not.

Comment 2

[Dão Gottwald [:dao]]

We should totally remove this.

Comment 3

[:Gijs (out for now; he/him)]

Dão, am I missing something or is this essentially a much better described duplicate of bug 958106? (I'd advocate for duping that here instead of vice versa, but there we are)

Comment 5

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #3)
> Dão, am I missing something or is this essentially a much better described
> duplicate of bug 958106? (I'd advocate for duping that here instead of vice
> versa, but there we are)

Yep.

Comment 6

[Dave Garrett]

https://mxr.mozilla.org/mozilla-central/search?string=star-button

Doesn't seem to be referenced anywhere else. Just need to delete 3 lines for the 1 element.

Comment 7

[:Gijs (out for now; he/him)]

I don't think this needs to be tracked under the add-ons tracker.

Comment 8

[Dave Garrett]

(In reply to :Gijs Kruitbosch from comment #7)
> I don't think this needs to be tracked under the add-ons tracker.

Why? Yes, it's relatively minor, but it directly affects multiple addons, including one with a lot of users. In addition to Flagfox, there's also things like SPDY Indicator.

Sure, I'm working around it in Flagfox 5.0b5, but this should still be fixed as soon as feasible. It will still affect current Flagfox 4.2.x users until the 5.0 release is ready (though, that will definitely be long before Australis makes it to final release). It probably also poses an obstacle for new Australis-supporting themes.

Comment 9

[:Gijs (out for now; he/him)]

(In reply to Dave Garrett from comment #8)
> (In reply to :Gijs Kruitbosch from comment #7)
> > I don't think this needs to be tracked under the add-ons tracker.
> 
> Why? Yes, it's relatively minor, but it directly affects multiple addons,
> including one with a lot of users. In addition to Flagfox, there's also
> things like SPDY Indicator.
> 
> Sure, I'm working around it in Flagfox 5.0b5, but this should still be fixed
> as soon as feasible. It will still affect current Flagfox 4.2.x users until
> the 5.0 release is ready (though, that will definitely be long before
> Australis makes it to final release). It probably also poses an obstacle for
> new Australis-supporting themes.

Is this just to avoid the extra empty space? That still doesn't sound like a major issue.

It sounds like *you* think it's a major issue though, and it sounds like this just involves deleting 3 lines of code - maybe you could just submit a patch here? :-)

Comment 10

[Dave Garrett]

I never said it was a major issue; just an issue that needs fixing.

In the amount of time it would take me to set up a build environment, figure out how to use hg, download trunk (assuming I actually have enough free space on this laptop at the moment), make and submit a patch, get it properly reviewed, and fix whatever mistake I make along the way, someone could've just deleted the 3 lines and committed it. I know there are other bugs of higher priority, but it would be nice if someone just cleaned up the leftover element if it's not supposed to be there anymore.

Comment 11

[Shubham Somani]

Attachment: 974229.patch

Removed the unused star button. Tested the changes using DOM Inspector. Seems to be ok. Please review. Thanks!

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8379918 [details] [diff] [review]
974229.patch

Looks good, thanks!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/2d851a8e2c67

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2d851a8e2c67

Comment 15

[:Gijs (out for now; he/him)]

Comment on attachment 8379918 [details] [diff] [review]
974229.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis / bug 755598
User impact if declined: users notice extra spacing in the URL bar, this can become more noticeable with certain add-ons
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low. If add-ons rely on the presence of the element, that could muck with things. However, so much is changing in Australis that this is extremely unlikely to be the breaking change amongst everything else that changed
String or IDL/UUID changes made by this patch: none

Comment 16

[:Gijs (out for now; he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/1a2c5e6bcb26