Restyle the Location bar

RESOLVED FIXED in Firefox 3.7a2

Status

()

Firefox
Theme
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

Trunk
Firefox 3.7a2
All
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 12 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 366142 [details] [diff] [review]
wip
(Assignee)

Updated

9 years ago
Depends on: 476738
(Assignee)

Comment 1

9 years ago
Created attachment 366143 [details] [diff] [review]
wip
Attachment #366142 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Blocks: 481382
(Assignee)

Updated

9 years ago
Blocks: 481858
(Assignee)

Comment 2

9 years ago
Created attachment 366167 [details] [diff] [review]
patch
Attachment #366143 - Attachment is obsolete: true
(Assignee)

Comment 3

9 years ago
Created attachment 366169 [details] [diff] [review]
patch
Attachment #366167 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #366169 - Flags: review?(mconnor)
(Assignee)

Updated

9 years ago
Blocks: 482105

Comment 4

9 years ago
+  -moz-box-shadow: 0 1px 1px rgba(0,0,0,.3) inset,
+                   0 0 1px #5390d2 inset,
+                   0 0 1px #5390d2,
+                   0 0 2px #5390d2,
+                   0 0 3px #5390d2,
+                   0 0 4px #5390d2,
+                   0 0 5px #5390d2,
+                   0 0 5px #5390d2;

Many box shadows = very expensive = probably not desired for chrome. Would using the spread radius (see the -moz-box-shadow docs) simplify this somewhat?
(Assignee)

Comment 5

9 years ago
I imagine that the expensiveness wouldn't slow down typing or autocomplete, would it? (Since it's the focused state that has many shadows.)
I'll look into using the spread radius anyway.
(Assignee)

Comment 6

9 years ago
Created attachment 366177 [details] [diff] [review]
patch v2

using spread radius
Attachment #366169 - Attachment is obsolete: true
Attachment #366177 - Flags: review?(mconnor)
Attachment #366169 - Flags: review?(mconnor)
(In reply to comment #4)
> Many box shadows = very expensive = probably not desired for chrome.

That's what had scared me away from trying this approach. But I really like
this patch, so I'll do some perf tests with my two-year-old Macbook.

(In reply to comment #5)
> I imagine that the expensiveness wouldn't slow down typing

Well, when you're typing, the field is focused... so whenever a new character
is painted, all the layers below it are painted too, and that includes the box
shadows (although the repainted rects are probably small enough to not matter). 

(Ideally, nsDisplayBoxShadowOuter::OptimizeVisibility would optimize this away,
but looking at the code it seems like that optimization won't occur because
you're using border-radius.)
(Assignee)

Comment 8

9 years ago
(In reply to comment #7)
> > I imagine that the expensiveness wouldn't slow down typing
> 
> Well, when you're typing, the field is focused...

Yeah, I didn't want to imply that this wouldn't be the interesting case, quite the contrary. But my assumption is that it wouldn't actually cause a lag when typing. At least I didn't notice it on my netbook.
Comment on attachment 366177 [details] [diff] [review]
patch v2

>+  -moz-border-top-colors: #666;
>+  -moz-border-right-colors: #777;
>+  -moz-border-bottom-colors: #777;
>+  -moz-border-left-colors: #777;

Why don't you use border-*-color here?

>+#identity-box:focus {

It feels like this has to be #urlbar[focused="true"] > #identity-box: If I focus the urlbar, click on the identity button and dismiss the panel by clicking anywhere else, #identity-box is still focused but the urlbar is not. (Maybe you can't get into this state on Windows because there the mouse click that dismisses the panel immediately focuses something else; on Mac that can't happen because there those clicks are eaten by the widget layer.)

Your patch introduces (at least) one visual difference: the focus ring no longer overlays the border. Could you get around this by rendering the border using a (0 0 0 1px) box shadow that's under the focus ring shadows?
(Assignee)

Comment 10

9 years ago
(In reply to comment #9)
> (From update of attachment 366177 [details] [diff] [review])
> >+  -moz-border-top-colors: #666;
> >+  -moz-border-right-colors: #777;
> >+  -moz-border-bottom-colors: #777;
> >+  -moz-border-left-colors: #777;
> 
> Why don't you use border-*-color here?

Because I need to override textbox.css.

> >+#identity-box:focus {
> 
> It feels like this has to be #urlbar[focused="true"] > #identity-box: If I
> focus the urlbar, click on the identity button and dismiss the panel by
> clicking anywhere else, #identity-box is still focused but the urlbar is not.

The use case this was introduced for is accel+L, shift+tab. This moves the focus to the button, and the focus ring shouldn't be missing in that case. #urlbar[focused="true"] > #identity-box is already handled elsewhere.

> Your patch introduces (at least) one visual difference: the focus ring no
> longer overlays the border. Could you get around this by rendering the border
> using a (0 0 0 1px) box shadow that's under the focus ring shadows?

I would think that the focus ring is too opaque there. But I could just change the border color for the focused state.
(In reply to comment #10)
> The use case this was introduced for is accel+L, shift+tab. This moves the
> focus to the button, and the focus ring shouldn't be missing in that case.

Ah, I see. Then we should probably use a different way of indicating focus there - right now, it looks like a bug because the focusring is broken on the right side. Maybe the focusring should only be around the favicon, not around the whole button?

> I would think that the focus ring is too opaque there.

Maybe add another, semi-opaque border box shadow on top of the focus ring shadows? Just an idea. :-)

> But I could just change
> the border color for the focused state.

Then graphite support wouldn't be a simple matter of replacing the hard-coded color with -moz-mac-focusring, would it?
(Assignee)

Comment 12

9 years ago
(In reply to comment #11)
> (In reply to comment #10)
> > The use case this was introduced for is accel+L, shift+tab. This moves the
> > focus to the button, and the focus ring shouldn't be missing in that case.
> 
> Ah, I see. Then we should probably use a different way of indicating focus
> there - right now, it looks like a bug because the focusring is broken on the
> right side. Maybe the focusring should only be around the favicon, not around
> the whole button?

Hm, I think a focus ring around the favicon wouldn't be ideal if the background is already blue (verified domain), but I'll have to test it. Maybe I could also fix the right side of the current focus ring.

> > But I could just change the border color for the focused state.
> 
> Then graphite support wouldn't be a simple matter of replacing the hard-coded
> color with -moz-mac-focusring, would it?

Right, I didn't think of that.
(Assignee)

Comment 13

9 years ago
(In reply to comment #12)
> Maybe I could also fix the right side of the current focus ring.

Well, I can't, because .textbox-input-box is anonymous and #identity-box is not, so #identity-box:focus + .textbox-input-box won't work.
(Assignee)

Comment 14

9 years ago
Created attachment 366209 [details] [diff] [review]
patch v3

this improves the border in the focused state and the focus ring of the identity box
Attachment #366177 - Attachment is obsolete: true
Attachment #366209 - Flags: review?(mconnor)
Attachment #366177 - Flags: review?(mconnor)
(Assignee)

Comment 15

9 years ago
Created attachment 366214 [details] [diff] [review]
patch v3.1

this is better, code-wise
Attachment #366209 - Attachment is obsolete: true
Attachment #366214 - Flags: review?(mconnor)
Attachment #366209 - Flags: review?(mconnor)
(Assignee)

Comment 16

9 years ago
(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 366177 [details] [diff] [review] [details])
> > >+  -moz-border-top-colors: #666;
> > >+  -moz-border-right-colors: #777;
> > >+  -moz-border-bottom-colors: #777;
> > >+  -moz-border-left-colors: #777;
> > 
> > Why don't you use border-*-color here?
> 
> Because I need to override textbox.css.

FYI, I've filed bug 482692 on that.
(Assignee)

Updated

9 years ago
Blocks: 455334
(Assignee)

Updated

9 years ago
Depends on: 481853
(Assignee)

Comment 17

9 years ago
Created attachment 368875 [details] [diff] [review]
patch v3.2

Now using -moz-mac-focusring, since bug 481853 is fixed. This will immediately fix bug 481382.
Attachment #366214 - Attachment is obsolete: true
Attachment #368875 - Flags: review?(mconnor)
Attachment #366214 - Flags: review?(mconnor)

Comment 18

9 years ago
(In reply to bug 455334 comment #52)
> I'll land this when bug 482086 is reviewed -- not sure how long that
> will take.

Not too long, hopefully :-) FWIW, when I was updating my patch for bug 455334, I tested patch v3.1 quite extensively (EV vs. non-EV, and different settings for identity.ssl_domain_display) - and did not encounter any unexpected behavior. In the meantime, I also applied v3.2, which works fine as well.
Hardware: x86 → All
Summary: Restyle the Location bar → [Mac] Restyle the Location bar
(Assignee)

Updated

9 years ago
Summary: [Mac] Restyle the Location bar → Restyle the Location bar
(Assignee)

Comment 19

9 years ago
Created attachment 371433 [details] [diff] [review]
patch v3.2

updated to latest trunk
Attachment #368875 - Attachment is obsolete: true
Attachment #371433 - Flags: review?(mconnor)
Attachment #368875 - Flags: review?(mconnor)
(Assignee)

Comment 20

9 years ago
Created attachment 374826 [details] [diff] [review]
patch v3.2

updated to latest trunk

Markus, do you want to review this?
Attachment #371433 - Attachment is obsolete: true
Attachment #374826 - Flags: review?(mstange)
Attachment #371433 - Flags: review?(mconnor)
I can do it.
Comment on attachment 374826 [details] [diff] [review]
patch v3.2

I think this is fine.

I just have two minor visual quibbles with it:
 - When the identity button is blue, there's a tiny quirk in its bottom left
   corner. It seems like the bottom border is bleeding upwards somehow.
 - In the focused state, the identity button's left border is a little too
   dark.
But neither of these quibbles seems to have a simple solution, so I think this is good to go. It will be interesting to see how Txul reacts to it.

The code cleanup is great!
Attachment #374826 - Flags: review?(mstange) → review+
(Assignee)

Updated

9 years ago
Blocks: 448922
(Assignee)

Comment 23

9 years ago
http://hg.mozilla.org/mozilla-central/rev/7015295489b0
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
Regression: Txul increase 6.02% on OSX 10.4

<http://graphs-new.mozilla.org/graph.html#tests=[{"machine":59,"test":17,"branch":1},{"machine":60,"test":17,"branch":1},{"machine":61,"test":17,"branch":1},{"machine":62,"test":17,"branch":1},{"machine":72,"test":17,"branch":1},{"machine":148,"test":17,"branch":1},{"machine":149,"test":17,"branch":1}]&sel=1244594520,1244767320> 

Previous results: 483.105 
from build 20090610164638 of revision ec7594969ed7 at 2009-06-10 16:46:00 on qm-pmac-fast03

New results: 512.211 
from build 20090610174222 of revision 0b7f20c1f468 at 2009-06-10 17:42:00 on qm-pmac-trunk01 run # 0

Suspected checkin range: 
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ec7594969ed7&tochange=0b7f20c1f468

... implicates this or bug 482105
... and a SunSpider regression:

http://graphs-new.mozilla.org/graph.html#tests=[{"machine":59,"test":25,"branch":1},{"machine":60,"test":25,"branch":1},{"machine":61,"test":25,"branch":1},{"machine":72,"test":25,"branch":1},{"machine":148,"test":25,"branch":1},{"machine":149,"test":25,"branch":1}]&sel=1244594520,1244767320
(Assignee)

Comment 26

9 years ago
backed out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 27

9 years ago
Created attachment 382675 [details] [diff] [review]
updated to trunk
Attachment #374826 - Attachment is obsolete: true
Is this a visual change without a ui-r, btw?
(Assignee)

Comment 29

9 years ago
no, it's refactoring.
Dão, what do you plan to do with this bug?
(Assignee)

Comment 31

8 years ago
I'm going to wait until Gecko can handle it. Do you have an alternative plan?
(Assignee)

Comment 32

8 years ago
Created attachment 400256 [details] [diff] [review]
updated to trunk
Attachment #382675 - Attachment is obsolete: true
Created attachment 428036 [details] [diff] [review]
updated to trunk
Attachment #400256 - Attachment is obsolete: true
Blocks: 547538
(In reply to comment #31)
> I'm going to wait until Gecko can handle it. Do you have an alternative plan?

I've profiled it and changed the CSS until the regression was gone.
See bug 547538.
http://hg.mozilla.org/mozilla-central/rev/63ec2ae4b651
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3.6a1 → Firefox 3.7a2
You need to log in before you can comment on or make changes to this bug.