The default bug view has changed. See this FAQ.

Allow tabs to have a visible label that is different from its actual label

REOPENED
Assigned to

Status

()

Firefox
Tabbed Browser
REOPENED
3 years ago
a year ago

People

(Reporter: ttaubert, Assigned: hobophobe)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 28
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
Needed as a prerequisite for bug 583890.
(Assignee)

Comment 1

3 years ago
Created attachment 8339633 [details] [diff] [review]
Moved test to its new bug number

This patch looks fine to me. Wish the other one were this simple.
Attachment #8339633 - Flags: review?(ttaubert)
Comment on attachment 8339633 [details] [diff] [review]
Moved test to its new bug number

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

::: browser/base/content/tabbrowser.xml
@@ +1553,5 @@
>              // initialized by this point.
>              this.mPanelContainer.appendChild(notificationbox);
>  
> +            // Happens after the browser is in the DOM: the TabTitleAbridger
> +            // classifies tabs by domains, requiring access to the browser.

It would be nice to have a test that ensures that the browser is accessible when the event is fired.

@@ +1554,5 @@
>              this.mPanelContainer.appendChild(notificationbox);
>  
> +            // Happens after the browser is in the DOM: the TabTitleAbridger
> +            // classifies tabs by domains, requiring access to the browser.
> +            if (!aURI || isBlankPageURL(aURI)) {

Drive-by feedback: I think the comment should be a bit more clear by indicating that consumers of TabLabelModified (such as TabTitleAbridger) may need access to the browser because as-is the comment didn't make sense to me since I didn't see the TabTitleAbridger in the patch and wondered what was calling it. A proper sentence would also be preferred.
Attachment #8339633 - Flags: feedback+
(Assignee)

Comment 3

3 years ago
Created attachment 8339678 [details] [diff] [review]
Improve comment, add linkedBrowser accessibility test

Thanks Matt!

The first few tests already fail if a new tab's label is set before the tab element is fully initialized, but the new test should defend the need for the linkedBrowser explicitly.
Attachment #8339633 - Attachment is obsolete: true
Attachment #8339633 - Flags: review?(ttaubert)
Attachment #8339678 - Flags: review?(ttaubert)
(Reporter)

Comment 4

3 years ago
Comment on attachment 8339678 [details] [diff] [review]
Improve comment, add linkedBrowser accessibility test

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

::: browser/base/content/tabbrowser.xml
@@ +1555,5 @@
>  
> +            // The label needs to be set after the browser is in the DOM.
> +            // This is because consumers of the TabLabelModified event
> +            // (such as TabTitleAbridger) need information that is with the
> +            // browser (such as the domain/URI of the tab).

Nit: don't mention the TTA, we're creating a generic API :)

And actually, we're not setting the label after the tab has been appended so the event handler can access the tab's browser but rather so that event can be dispatched at all and will bubble up to any event handler registered on the tabContainer.

@@ +4692,5 @@
> +          });
> +          this.dispatchEvent(event);
> +
> +          if (!event.defaultPrevented)
> +            this.visibleLabel = val;

Please add a comment to say that we're allowing listeners to cancel overriding a custom tab label.

::: browser/base/content/test/general/browser.ini
@@ +230,5 @@
>  [browser_bug887515.js]
>  [browser_bug896291_closeMaxSessionStoreTabs.js]
>  [browser_bug902156.js]
>  [browser_bug906190.js]
> +[browser_bug943820.js]

The test should have a better name, browser_visibleLabel.js maybe?

::: browser/base/content/test/general/browser_bug943820.js
@@ +12,5 @@
> +  waitForExplicitFinish();
> +  registerCleanupFunction(function() {
> +    gBrowser.removeCurrentTab({animate: false});
> +  });
> +  let tab = gBrowser.addTab("about:newtab",

I'd rather use about:blank here, about:newtab may not fire a load event at all when it was preloaded.

@@ +16,5 @@
> +  let tab = gBrowser.addTab("about:newtab",
> +                            {skipAnimation: true});
> +  tab.linkedBrowser.addEventListener("load", function onLoad(event) {
> +    event.currentTarget.removeEventListener("load", onLoad, true);
> +    gBrowser.selectedTab = tab;

You can do |let tab = gBrowser.selectedTab = gBrowser.addTab(...)|.

@@ +23,5 @@
> +  tab.addEventListener("TabLabelModified", handleInTest, true);
> +}
> +
> +// Prevent interference
> +function handleInTest(aEvent) {

When landed without the TabTitleAbridger this doesn't really make sense. There is nothing that interferes here (yet) and we always set visibleLabel=label anyway. Once bug 583890 lands it should probably fix this test by disabling itself for the runtime of the test.

@@ +66,5 @@
> +  tab.addEventListener("TabLabelModified", handleTabLabel, true);
> +  tab.label = "This won't be the visibleLabel";
> +}
> +
> +function handleTabLabel(aEvent) {

Would "overrideTabLabel" be a better name?

@@ +93,5 @@
> +
> +function checkBrowserAccessibilityOnNewTab() {
> +  gBrowser.tabContainer.addEventListener("TabLabelModified",
> +    handleBrowserAccessibilityTest, true);
> +  let tab = gBrowser.addTab("about:newtab", {skipAnimation: true});

about:blank

@@ +100,5 @@
> +function handleBrowserAccessibilityTest(aEvent) {
> +  gBrowser.tabContainer.removeEventListener("TabLabelModified",
> +    handleBrowserAccessibilityTest, true);
> +  ok(aEvent.target.linkedBrowser,
> +    "Linked browser is accessible by the event handler");

That check seems superfluous. The event was dispatched and handled so we know the tab was in the tree before it fired the event.
Attachment #8339678 - Flags: review?(ttaubert) → feedback+
(Assignee)

Comment 5

3 years ago
Created attachment 8340147 [details] [diff] [review]
Update comments, use about:blank in tests, address other feedback

Thanks, Tim!

> @@ +100,5 @@
> > +function handleBrowserAccessibilityTest(aEvent) {
> > +  gBrowser.tabContainer.removeEventListener("TabLabelModified",
> > +    handleBrowserAccessibilityTest, true);
> > +  ok(aEvent.target.linkedBrowser,
> > +    "Linked browser is accessible by the event handler");
> 
> That check seems superfluous. The event was dispatched and handled so we know
> the tab was in the tree before it fired the event.

I've replaced it with an |ok(true, ...);| to note success there.
Attachment #8339678 - Attachment is obsolete: true
Attachment #8340147 - Flags: review?(ttaubert)
(Reporter)

Comment 6

3 years ago
Comment on attachment 8340147 [details] [diff] [review]
Update comments, use about:blank in tests, address other feedback

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

Looks good to me, thanks! Did you push it to try yet?
Attachment #8340147 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 7

3 years ago
Created attachment 8340164 [details] [diff] [review]
Prepared for checkin

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=d76973f66170
Attachment #8340147 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Reporter)

Comment 8

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/089349127563
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/089349127563
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28

Comment 10

3 years ago
browser/base/content/test/general/browser_visibleLabel.js
Flags: in-testsuite+
Depends on: 1247920
I backed this out in bug 1247920.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
You need to log in before you can comment on or make changes to this bug.