Closed Bug 482086 Opened 12 years ago Closed 11 years ago

Restyle the Location bar

Categories

(Firefox :: Theme, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3.7a2

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file, 12 obsolete files)

Attached patch wip (obsolete) — Splinter Review
No description provided.
Depends on: 476738
Attached patch wip (obsolete) — Splinter Review
Attachment #366142 - Attachment is obsolete: true
Blocks: 481382
Blocks: 481858
Attached patch patch (obsolete) — Splinter Review
Attachment #366143 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Attachment #366167 - Attachment is obsolete: true
Attachment #366169 - Flags: review?(mconnor)
Blocks: 482105
+  -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?
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.
Attached patch patch v2 (obsolete) — Splinter Review
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.)
(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?
(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?
(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.
(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.
Attached patch patch v3 (obsolete) — Splinter Review
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)
Attached patch patch v3.1 (obsolete) — Splinter Review
this is better, code-wise
Attachment #366209 - Attachment is obsolete: true
Attachment #366214 - Flags: review?(mconnor)
Attachment #366209 - Flags: review?(mconnor)
(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.
Blocks: 455334
Depends on: 481853
Attached patch patch v3.2 (obsolete) — Splinter Review
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)
(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
Summary: [Mac] Restyle the Location bar → Restyle the Location bar
Attached patch patch v3.2 (obsolete) — Splinter Review
updated to latest trunk
Attachment #368875 - Attachment is obsolete: true
Attachment #371433 - Flags: review?(mconnor)
Attachment #368875 - Flags: review?(mconnor)
Attached patch patch v3.2 (obsolete) — Splinter Review
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+
Blocks: 448922
http://hg.mozilla.org/mozilla-central/rev/7015295489b0
Status: ASSIGNED → RESOLVED
Closed: 11 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
backed out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch updated to trunk (obsolete) — Splinter Review
Attachment #374826 - Attachment is obsolete: true
Is this a visual change without a ui-r, btw?
no, it's refactoring.
Dão, what do you plan to do with this bug?
I'm going to wait until Gecko can handle it. Do you have an alternative plan?
Attached patch updated to trunk (obsolete) — Splinter Review
Attachment #382675 - Attachment is obsolete: true
Attached patch updated to trunkSplinter Review
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
Closed: 11 years ago11 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.