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

VERIFIED FIXED in Firefox 4.0b11

Status

()

Firefox
Address Bar
VERIFIED FIXED
7 years ago
4 years ago

People

(Reporter: Matheus Kerschbaum, Assigned: fryn)

Tracking

({polish})

Trunk
Firefox 4.0b11
polish
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

(Whiteboard: [switch-to-tab])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Blocks: 587908
blocking2.0: --- → ?
Version: unspecified → Trunk
I cant reproduce it on Win7.
Confirming, thanks for filing.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Summary: Switch-to-tab shows bogus moz-action:switchtab when hovering links → Hovering links shows "moz-action:switchtab" text and hides "Switch to tab:" label in URL bar when it contains a switch-to-tab URL
Janky. Nice find.
blocking2.0: ? → final+
Duplicate of this bug: 609997
(Assignee)

Updated

7 years ago
Assignee: nobody → fryn
(Assignee)

Comment 6

7 years ago
(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?
(Assignee)

Comment 7

7 years ago
Created attachment 490727 [details] [diff] [review]
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.
Attachment #490727 - Flags: review?(dao)
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
(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 on attachment 490727 [details] [diff] [review]
patch

This increases the location bar height when "Switch to tab" is displayed. Tested on Linux.
Attachment #490727 - Flags: review?(dao) → review-
(Assignee)

Comment 10

7 years ago
Created attachment 493891 [details] [diff] [review]
patch v2

fixed margins on Windows and Linux.
Attachment #490727 - Attachment is obsolete: true
Attachment #493891 - Flags: review?(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
Attachment #493891 - Flags: review?(dao) → review-
(Assignee)

Comment 12

7 years ago
Created attachment 495172 [details] [diff] [review]
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.
Attachment #493891 - Attachment is obsolete: true
Attachment #495172 - Flags: review?(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.

Updated

7 years ago
Attachment #495172 - Flags: review?(dao) → review-
blocking2.0: final+ → -
Keywords: polish
(Assignee)

Comment 14

7 years ago
Created attachment 498027 [details] [diff] [review]
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.
Attachment #495172 - Attachment is obsolete: true
Attachment #498027 - Flags: review?(dao)
(Assignee)

Comment 15

7 years ago
Created attachment 498030 [details] [diff] [review]
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
Attachment #498027 - Attachment is obsolete: true
Attachment #498030 - Flags: review?(dao)
Attachment #498027 - Flags: review?(dao)
(Assignee)

Comment 16

7 years ago
Created attachment 498158 [details] [diff] [review]
patch v6 (updated to tip of tree)
Attachment #498030 - Attachment is obsolete: true
Attachment #498158 - Flags: review?(dao)
Attachment #498030 - Flags: review?(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.
Attachment #498158 - Flags: review?(dao) → review-
(Assignee)

Comment 18

7 years ago
(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.
You seem to be talking about the margin of the box.
(Assignee)

Comment 20

7 years ago
Created attachment 503015 [details] [diff] [review]
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.
Attachment #498158 - Attachment is obsolete: true
Attachment #503015 - Flags: review?(dao)

Updated

7 years ago
Attachment #503015 - Flags: review?(dao) → review+
(Assignee)

Updated

7 years ago
Attachment #503015 - Flags: approval2.0?
Attachment #503015 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/0718ec141474
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Target Milestone: --- → Firefox 4.0b11

Updated

7 years ago
Depends on: 631184
Whiteboard: [switch-to-tab]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.