Closed Bug 974229 Opened 6 years ago Closed 6 years ago

Remove obsolete star-button element

Categories

(Firefox :: Address Bar, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: davemgarrett, Assigned: shubhamsomani92)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P4][good first bug][lang=xul][mentor=dao])

Attachments

(1 file)

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.
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.
We should totally remove this.
Component: Theme → Location Bar
Summary: star-button shim in the address bar still affects layout → Remove obsolete star-button element
Whiteboard: [good first bug][lang=xul][mentor=dao]
Version: 29 Branch → Trunk
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)
Flags: needinfo?(dao)
Duplicate of this bug: 958106
(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.
Flags: needinfo?(dao)
Whiteboard: [good first bug][lang=xul][mentor=dao] → [Australis:P4][good first bug][lang=xul][mentor=dao]
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.
I don't think this needs to be tracked under the add-ons tracker.
No longer blocks: australis-addons
(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.
(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? :-)
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.
Attached patch 974229.patchSplinter Review
Removed the unused star button. Tested the changes using DOM Inspector. Seems to be ok. Please review. Thanks!
Attachment #8379918 - Flags: feedback?(dao)
Comment on attachment 8379918 [details] [diff] [review]
974229.patch

Looks good, thanks!
Attachment #8379918 - Flags: feedback?(dao) → review+
Assignee: nobody → shubhamsomani92
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/2d851a8e2c67
Keywords: checkin-needed
Whiteboard: [Australis:P4][good first bug][lang=xul][mentor=dao] → [Australis:P4][good first bug][lang=xul][mentor=dao][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2d851a8e2c67
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P4][good first bug][lang=xul][mentor=dao][fixed-in-fx-team] → [Australis:P4][good first bug][lang=xul][mentor=dao]
Target Milestone: --- → Firefox 30
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
Attachment #8379918 - Flags: approval-mozilla-aurora?
Attachment #8379918 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.