Closed Bug 1059600 Opened 10 years ago Closed 9 years ago

New tabs for javascript: links don't have a title

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 37
Iteration:
37.1
Tracking Status
firefox32 --- unaffected
firefox33 --- wontfix
firefox34 --- wontfix
firefox35 --- verified
firefox36 --- verified
firefox37 --- verified

People

(Reporter: bnicholson, Assigned: dao)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

STR:
1) Go to data:text/html,<a href="javascript:void()">foo</a>
2) ctrl+click the link

Previously, this would open a tab with the JS URI as the title. Now, it simply opens a tab with no title, resulting in an empty space in the tabs panel that looks broken.

Regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a6d4659e1031&tochange=89cb4420380d

MattN thinks this is from bug 1017156, so blocking that.
Flags: qe-verify?
Flags: firefox-backlog+
Points: --- → 2
Flags: qe-verify? → qe-verify+
Marco, can you add this to the prioritized list for the next iteration? Thanks!
Flags: needinfo?(mmucci)
Added to IT 37.1 priority list as new work.
Flags: needinfo?(mmucci)
Assignee: nobody → dao
Status: NEW → ASSIGNED
Iteration: --- → 37.1
Attached patch patch (obsolete) — Splinter Review
Trivial stop-gap solution. This does not fix bug 1085197. That will be more involved.
Attachment #8531702 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8531702 [details] [diff] [review]
patch

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

Is this better, in the end, than just backing out 1017156 and fixing 1085197 instead?

::: browser/base/content/tabbrowser.xml
@@ +1658,5 @@
>              // We've waited until the tab is in the DOM to set the label. This
>              // allows the TabLabelModified event to be properly dispatched.
>              if (!aURI || isBlankPageURL(aURI)) {
>                t.label = this.mStringBundle.getString("tabs.emptyTabTitle");
> +            } else if (aURI.toLowerCase().startsWith("javascript:")) {

Assuming you think the answer is 'no', should we do this for any other protocols, offhand?
Attachment #8531702 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #5)
> Comment on attachment 8531702 [details] [diff] [review]
> patch
> 
> Review of attachment 8531702 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is this better, in the end, than just backing out 1017156 and fixing 1085197
> instead?

Well, bug 1085197 isn't an issue without bug 1017156, so this question doesn't make much sense to me. Anyway, I don't think bug 1017156 should be backed out because of bug 1085197.

> ::: browser/base/content/tabbrowser.xml
> @@ +1658,5 @@
> >              // We've waited until the tab is in the DOM to set the label. This
> >              // allows the TabLabelModified event to be properly dispatched.
> >              if (!aURI || isBlankPageURL(aURI)) {
> >                t.label = this.mStringBundle.getString("tabs.emptyTabTitle");
> > +            } else if (aURI.toLowerCase().startsWith("javascript:")) {
> 
> Assuming you think the answer is 'no', should we do this for any other
> protocols, offhand?

Not that I know.
(In reply to Dão Gottwald [:dao] from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > Comment on attachment 8531702 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 8531702 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Is this better, in the end, than just backing out 1017156 and fixing 1085197
> > instead?
> 
> Well, bug 1085197 isn't an issue without bug 1017156, so this question
> doesn't make much sense to me. Anyway, I don't think bug 1017156 should be
> backed out because of bug 1085197.

Right, I meant backing out bug 1017156 until we can reland it with both this and your suggestion in the last comment on bug 1085197 fixed. Anyway, if you think bug 1085197 isn't severe enough to bother backing anything out, then carry on. :-)
Not obvious enough for me to see why, but https://treeherder.mozilla.org/logviewer.html#?job_id=1355406&repo=fx-team, "browser_bug906190.js | uncaught exception - TypeError: aURI.toLowerCase is not a function at chrome://browser/content/tabbrowser.xml:1658". Backed out in https://hg.mozilla.org/integration/fx-team/rev/cb8849c52508.
browser_bug906190.js is off the charts. I don't know how this ever worked.

https://hg.mozilla.org/integration/fx-team/rev/9a2c20e8fe59
Attachment #8531702 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/9a2c20e8fe59
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
I was able to reproduce this issue on Firefox 34.0a1 (2014-08-27) using Windows 7 x64.

Verified fixed on Latest Firefox 37.0a1 (2014-12-07) with e10s enabled and disabled using Windows 7 x64, Ubuntu 14.04 x86 and Mac OSX 10.10.
Status: RESOLVED → VERIFIED
Comment on attachment 8532566 [details] [diff] [review]
patch, browser_bug906190.js fixed

Approval Request Comment
[Feature/regressing bug #]: bug 1017156
[User impact if declined]: see comment 0
[Describe test coverage new/current, TBPL]: this particular edge case doesn't have tests
[Risks and why]: conservative fix, low risk
[String/UUID change made/needed]: none
Attachment #8532566 - Flags: approval-mozilla-beta?
Attachment #8532566 - Flags: approval-mozilla-aurora?
Attachment #8532566 - Flags: approval-mozilla-beta?
Attachment #8532566 - Flags: approval-mozilla-beta+
Attachment #8532566 - Flags: approval-mozilla-aurora?
Attachment #8532566 - Flags: approval-mozilla-aurora+
QA Contact: vasilica.mihasca
Verified fixed on Latest Firefox 36.0a2 (2014-12-11) and Firefox 35 Beta 3 (20141211142524) using Windows 7 x64, Ubuntu 14.04 x86 and Mac OSX 10.9.5
You need to log in before you can comment on or make changes to this bug.