482086 - Restyle the Location bar

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Dão Gottwald [:dao]]

Attachment: wip

Comment 1

[Dão Gottwald [:dao]]

Attachment: wip

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch

Comment 4

[Michael Ventnor]

+  -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?

Comment 5

[Dão Gottwald [:dao]]

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.

Comment 6

[Dão Gottwald [:dao]]

Attachment: patch v2

using spread radius

Comment 7

[Markus Stange [:mstange]]

(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.)

Comment 8

[Dão Gottwald [:dao]]

(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 9

[Markus Stange [:mstange]]

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?

Comment 10

[Dão Gottwald [:dao]]

(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.

Comment 11

[Markus Stange [:mstange]]

(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?

Comment 12

[Dão Gottwald [:dao]]

(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.

Comment 13

[Dão Gottwald [:dao]]

(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.

Comment 14

[Dão Gottwald [:dao]]

Attachment: patch v3

this improves the border in the focused state and the focus ring of the identity box

Comment 15

[Dão Gottwald [:dao]]

Attachment: patch v3.1

this is better, code-wise

Comment 16

[Dão Gottwald [:dao]]

(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.

Comment 17

[Dão Gottwald [:dao]]

Attachment: patch v3.2

Now using -moz-mac-focusring, since bug 481853 is fixed. This will immediately fix bug 481382.

Comment 18

[Kaspar]

(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.

Comment 19

[Dão Gottwald [:dao]]

Attachment: patch v3.2

updated to latest trunk

Comment 20

[Dão Gottwald [:dao]]

Attachment: patch v3.2

updated to latest trunk

Markus, do you want to review this?

Comment 21

[Markus Stange [:mstange]]

I can do it.

Comment 22

[Markus Stange [:mstange]]

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!

Comment 23

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/7015295489b0

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

... 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

Comment 26

[Dão Gottwald [:dao]]

backed out

Comment 27

[Dão Gottwald [:dao]]

Attachment: updated to trunk

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Is this a visual change without a ui-r, btw?

Comment 29

[Dão Gottwald [:dao]]

no, it's refactoring.

Comment 30

[Markus Stange [:mstange]]

Dão, what do you plan to do with this bug?

Comment 31

[Dão Gottwald [:dao]]

I'm going to wait until Gecko can handle it. Do you have an alternative plan?

Comment 32

[Dão Gottwald [:dao]]

Attachment: updated to trunk

Comment 33

[Markus Stange [:mstange]]

Attachment: updated to trunk

Comment 34

[Markus Stange [:mstange]]

(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.

Comment 35

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/63ec2ae4b651