The default bug view has changed. See this FAQ.

Hide UI for toggling tabs on top unless the user disabled tabs on top

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Menus
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

Trunk
Firefox 15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 624318 [details] [diff] [review]
patch

see bug 755593 comment 1
Attachment #624318 - Flags: review?(dolske)

Comment 1

5 years ago
Comment on attachment 624318 [details] [diff] [review]
patch

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

::: browser/base/content/browser.js
@@ +5467,5 @@
>      Services.prefs.addObserver(this._prefName, this, false);
> +
> +    // Only show the toggle UI if the user disabled tabs on top.
> +    if (Services.prefs.getBoolPref(this._prefName)) {
> +      for (item of document.querySelectorAll("menuitem[command=cmd_ToggleTabsOnTop]"))

Use `let` here.

What is the benefit of using a `for` loop here that runs in the common case (of [tabsontop=true]) instead of having hidden=true on the <menuitem/>s in the markup by default.
Attachment #624318 - Flags: review?(dolske) → review?(gavin.sharp)

Comment 2

5 years ago
I moved the review flag to Gavin, since he showed interest in getting this change in soon.
Also, why not just give the menu item an ID and access it directly?
(Assignee)

Comment 4

5 years ago
We have three cmd_ToggleTabsOnTop menu items.

Comment 5

5 years ago
(In reply to Dão Gottwald [:dao] from comment #4)

That doesn't answer my question, which could be rephrased as: is it better to have a <menuitem/> removed from the document at load than have it be declared as [hidden=true]?
(Assignee)

Comment 6

5 years ago
(In reply to Frank Yan (:fryn) from comment #5)
> That doesn't answer my question,

I replied to comment 3.

> which could be rephrased as: is it better
> to have a <menuitem/> removed from the document at load than have it be
> declared as [hidden=true]?

I expect querySelectorAll to be cheap, so I don't think it matters.

Comment 7

5 years ago
(In reply to Dão Gottwald [:dao] from comment #6)

Okay, but given that the tabs-on-top is the default, we should initialize our UI in a state that reflects that, i.e. flipping the `if` condition and setting [hidden=true] in the markup. Dolske recommended this kind of thing when I first wrote the combined stop/go/reload code. See bug 544816 comment 54.
(Assignee)

Comment 8

5 years ago
It's more relevant for any content rendered by Gecko.

Comment 9

5 years ago
Comment on attachment 624318 [details] [diff] [review]
patch

(In reply to Dão Gottwald [:dao] from comment #8)
> It's more relevant for any content rendered by Gecko.

Good point.

feedback+ with the `let` added.
Attachment #624318 - Flags: feedback+
(In reply to Frank Yan (:fryn) from comment #5)

> That doesn't answer my question, which could be rephrased as: is it better
> to have a <menuitem/> removed from the document at load than have it be
> declared as [hidden=true]?

As a general rule I'd probably suggest using hidden=true, since that's what we do with other menus (most infamously the main context menu :). But since we're talking about actually _removing_ this at some point, we might as well just start doing so now. (EG, if an add-on makes use of it, it's gonna have to change sooner or later).
Comment on attachment 624318 [details] [diff] [review]
patch

(In reply to Frank Yan (:fryn) from comment #2)
> I moved the review flag to Gavin, since he showed interest in getting this
> change in soon.

*yoink* :)
Attachment #624318 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 12

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/89227104913a
Target Milestone: --- → Firefox 15
https://hg.mozilla.org/mozilla-central/rev/89227104913a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 763294

Comment 14

4 years ago
Why you doing this?

I don't like that the tabs are on top (over the address bar) because it makes the disctance you have to move the mouse longer and this makes it for me less efficient to use the browser.

So, please bring at least the option back to set the tabs under the address bar.
You need to log in before you can comment on or make changes to this bug.