601060 - Hovering links shows "moz-action:switchtab" text and hides "Switch to tab:" label in URL bar when it contains a switch-to-tab URL

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Matheus Kerschbaum]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b7pre) Gecko/20100930 Firefox/4.0b7pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b7pre) Gecko/20100930 Firefox/4.0b7pre

Looks like the "Switch to tab" message doesn't play nicely with link previews.

Reproducible: Always

Steps to Reproduce:
1.Select an open tab using the keyboard so that the "Switch to tab | http://example.com" message appears
2.Hover a link
Actual Results:  
Awesomebar shows the current URL as moz-action:switchtab, http://example.com

Expected Results:  
Current URL should be displayed correctly.

Comment 1

[Csaba Kozák [:WonderCsabo]]

I cant reproduce it on Win7.

Comment 2

[Drew Willcoxon :adw]

Confirming, thanks for filing.

Comment 3

[Drew Willcoxon :adw]

The "moz-action" text part is really easy to fix: _updateOverLink in urlbarBindings.xml should use this._value, which is basically the the URL bar's value to the outside world, rather than this.value, which can include the moz-action prefix.

But the "Switch to tab:" label shouldn't be hidden, and that will take some rearranging of the binding's children.

Comment 4

[Johnathan Nightingale [:johnath]]

Janky. Nice find.

Comment 6

[Frank Yan (:fryn)]

(In reply to comment #3)
> The "moz-action" text part is really easy to fix: _updateOverLink in
> urlbarBindings.xml should use this._value, which is basically the the URL bar's
> value to the outside world, rather than this.value, which can include the
> moz-action prefix.

Changing this.value to this._value didn't fix the bug. It seems that the url bar's _value property is being modified to match its value property before the following line runs:
          this._originLabel.value = this.value;

What does fix it is using:
          this._originLabel.value = this._parseActionUrl(this.value).param;

adw, any thoughts?

Comment 7

[Frank Yan (:fryn)]

Attachment: patch

Fixes the -moz-action bit using the above method.

Unhides the label with a trivial change to the binding.

I don't think this requires ui-review, but if it does, let me know.

Comment 8

[Drew Willcoxon :adw]

(In reply to comment #6)
> adw, any thoughts?

(In reply to comment #7)
> Unhides the label with a trivial change to the binding.

Nice!

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 490727 [details] [diff] [review]
patch

This increases the location bar height when "Switch to tab" is displayed. Tested on Linux.

Comment 10

[Frank Yan (:fryn)]

Attachment: patch v2

fixed margins on Windows and Linux.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 493891 [details] [diff] [review]
patch v2

With this, "Switch to tab" is vertically misaligned on Windows, and I'm getting this error a lot:

Error: this._parseActionUrl(this._value) is null
Source File: chrome://browser/content/urlbarBindings.xml
Line: 737

Comment 12

[Frank Yan (:fryn)]

Attachment: patch v3

On Windows 7, the 1-pixel misalignment only occurred in Windows classic (not basic, not aero), and I tracked it to a difference in the padding between the windows themes for html|input:
https://mxr.mozilla.org/mozilla-central/source/layout/reftests/editor/xul/input.css#15

Fixed it using :-moz-system-metric.

Comment 13

[Dão Gottwald [:dao]]

(In reply to comment #12)
> On Windows 7, the 1-pixel misalignment only occurred in Windows classic (not
> basic, not aero), and I tracked it to a difference in the padding between the
> windows themes for html|input:
> https://mxr.mozilla.org/mozilla-central/source/layout/reftests/editor/xul/input.css#15

Still seeing it on XP. I don't see why that file is relevant here, it's only used in reftests.

Comment 14

[Frank Yan (:fryn)]

Attachment: patch v4

Wraps the <label> in a <box align="center"> for consistent styling on Windows.
Due to padding on the url bar, we still need some negative margins, but it's still much better than before.

Comment 15

[Frank Yan (:fryn)]

Attachment: patch v5

my bad: forgot to add padding-top: 1px; to pinstripe/.../browser.css

tested on ubuntu 10.10, windows xp, windows 7 aero + aero basic + classic, and os x 10.6

Comment 16

[Frank Yan (:fryn)]

Attachment: patch v6 (updated to tip of tree)

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 498158 [details] [diff] [review]
patch v6 (updated to tip of tree)

The 1px top padding is likely only needed because the label's vertical margins are uneven. Seems like these should be set to 0.

Comment 18

[Frank Yan (:fryn)]

(In reply to comment #17)
> Comment on attachment 498158 [details] [diff] [review]
> patch v6 (updated to tip of tree)
> 
> The 1px top padding is likely only needed because the label's vertical margins
> are uneven. Seems like these should be set to 0.

The uneven vertical margins are there to align the top and bottom of the right border with the urlbar border. Setting these all to 0 will break that. I haven't found a more elegant way to do this. You can give it a shot.

Comment 19

[Dão Gottwald [:dao]]

You seem to be talking about the margin of the box.

Comment 20

[Frank Yan (:fryn)]

Attachment: patch v7

(In reply to comment #17)
> Comment on attachment 498158 [details] [diff] [review]
> patch v6 (updated to tip of tree)
> 
> The 1px top padding is likely only needed because the label's vertical margins
> are uneven. Seems like these should be set to 0.

Addressed and tested on OS X, Ubuntu, Windows 7 (Aero Glass, Aero Basic, Windows Classic), and Windows XP.

Comment 21

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/0718ec141474