Closed
Bug 997131
Opened 11 years ago
Closed 11 years ago
Simplify conditional forward button CSS by making the forward button a direct sibling of the location bar
Categories
(Firefox :: Theme, defect)
Firefox
Theme
Tracking
()
RESOLVED
FIXED
Firefox 31
People
(Reporter: dao, Assigned: dao)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [Australis:P3+][qa-])
Attachments
(5 files, 6 obsolete files)
This works on Linux, haven't tested it elsewhere yet.
Other than simplifying the code, this should pave the way for addressing bug 995300.
Assignee | ||
Comment 1•11 years ago
|
||
Comment on attachment 8407483 [details] [diff] [review]
WIP patch
Works fine on Windows. Mike, would you mind giving this a try on OS X? If it works, feel free to do a full review.
Attachment #8407483 -
Flags: feedback?(mdeboer)
Assignee | ||
Comment 2•11 years ago
|
||
Hmm, seems like sharing the Windows and Linux clip path with OS X won't quite work, at least not without changing the clip path, which I'd leave to a different bug...
Assignee | ||
Updated•11 years ago
|
Attachment #8407483 -
Flags: feedback?(mdeboer)
Assignee | ||
Comment 3•11 years ago
|
||
this should be better on OS X, although it might still have issues
Attachment #8407483 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8407676 -
Flags: review?(mdeboer)
Comment 4•11 years ago
|
||
Comment on attachment 8407676 [details] [diff] [review]
WIP patch 2
Review of attachment 8407676 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/base/content/browser.xul
@@ -662,5 @@
> - cui-areatype="toolbar"
> - onclick="checkForMiddleClick(this, event);"
> - tooltip="forward-button-tooltip"
> - context="backForwardMenu"/>
> - <dummyobservertarget hidden="true"
the <dummyobservertarget> element was put there before 'my time' and I'd like to understand its purpose; why was it here for and why can it be removed at this time?
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #4)
> the <dummyobservertarget> element was put there before 'my time' and I'd
> like to understand its purpose; why was it here for and why can it be
> removed at this time?
It sets / removes the forwarddisabled attribute. I don't need that attribute anymore.
E.g.
@conditionalForwardWithUrlbar@[forwarddisabled] > #urlbar-wrapper > #urlbar
becomes
@conditionalForwardWithUrlbar@ > #forward-button[disabled] + #urlbar
Updated•11 years ago
|
Whiteboard: [Australis:P5]
Comment 6•11 years ago
|
||
Comment on attachment 8407676 [details] [diff] [review]
WIP patch 2
Review of attachment 8407676 [details] [diff] [review]:
-----------------------------------------------------------------
What I see on Windows XP/7/8 and Linux is that the urlbar (border) is offset by 1px from the back-button. This happens in both LTR and RTL mode, forward button disabled. This probably means that the applied clip-path needs to move 1px to the start.
Other than that, this path looks great! I really like how you indeed simplify things here. Once you fixed the clippath offsets, this one will be ready to land.
One last thing I'd like to get out of the way; can you check/ make sure that this doesn't cause a Talos regression? (I'm thinking TART & tresize primarily.)
Attachment #8407676 -
Flags: review?(mdeboer) → review-
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #6)
> Comment on attachment 8407676 [details] [diff] [review]
> WIP patch 2
>
> Review of attachment 8407676 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> What I see on Windows XP/7/8 and Linux is that the urlbar (border) is offset
> by 1px from the back-button.
If I understand you correctly, this part looked fine to me. Can you attach a screenshot?
Comment 8•11 years ago
|
||
Comment 9•11 years ago
|
||
Comment 10•11 years ago
|
||
Assignee | ||
Comment 11•11 years ago
|
||
This should be better.
If it's still off on Windows 8 (I've only checked 7), then that's due to the back button having a different size. That would be bug 998157.
As for the Gnome default theme (I've only checked the Ubuntu default theme), it makes the location bar one pixel taller due to font differences as you know. The clip path however doesn't grow with the location bar, so there might still be a small glitch at the bottom border. That's also orthogonal to this bug.
Attachment #8407676 -
Attachment is obsolete: true
Attachment #8410902 -
Flags: review?(mdeboer)
Comment 12•11 years ago
|
||
Dão, I didn't catch this earlier, but the OSX button looks offset by 1px too...
I'll take a look quickly as well, see if I can easily fix this.
Comment 13•11 years ago
|
||
Setting
```css
#back-button {
-moz-margin-end: 1px;
}
```
does the trick.
Next problem: on OSX, in RTL mode, the end border of the forward-button is not visible.
Comment 14•11 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #13)
> ```css
> #back-button {
> -moz-margin-end: 1px;
> }
> ```
or setting it to `0` and setting the x-offset of the clip-path to 11 looks even better.
Comment 15•11 years ago
|
||
Comment on attachment 8410902 [details] [diff] [review]
patch
Review of attachment 8410902 [details] [diff] [review]:
-----------------------------------------------------------------
I can confirm this looks a-ok on Linux & Windows, all versions under all circumstances I can test; different themes, OS themes, OS font-size settings, etc.
Only platform left to fix: OSX. See comment 12 and comment 14.
Once you're able to fix that last bit, r=me!
Attachment #8410902 -
Flags: review?(mdeboer) → review-
Comment 16•11 years ago
|
||
Upping prio, because of the bugs it blocks. It's also a shame that we can't uplift this to beta anymore...
status-firefox29:
--- → wontfix
status-firefox30:
--- → affected
status-firefox31:
--- → affected
Whiteboard: [Australis:P5] → [Australis:P3+]
Assignee | ||
Comment 17•11 years ago
|
||
Like this?
(In reply to Mike de Boer [:mikedeboer] from comment #13)
> Setting
>
> ```css
> #back-button {
> -moz-margin-end: 1px;
> }
> ```
>
> does the trick.
>
> Next problem: on OSX, in RTL mode, the end border of the forward-button is
> not visible.
Did this become a problem with the negative margin applied or was it already a problem without that?
Attachment #8410902 -
Attachment is obsolete: true
Attachment #8410992 -
Flags: review?(mdeboer)
Comment 18•11 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #17)
> Created attachment 8410992 [details] [diff] [review]
> patch v2
>
> Like this?
Yup!
> Did this become a problem with the negative margin applied or was it already
> a problem without that?
No, it was already a problem without that.
Only problem left now is is the missing end border of the forward button on OSX in RTL mode.
Comment 19•11 years ago
|
||
To fix the problem with the missing border you must do:
```css
#forward-button {
margin-left: -2px;
margin-right: 0;
padding-left: 2px;
width: 32px;
}
```
Instead what's currently
```css
#forward-button {
-moz-margin-start: -2px;
-moz-margin-end: 0;
-moz-padding-start: 2px;
width: 32px;
}
```
(You're transforming the #forward-button container now, so you need to use regular CSS positioning props)
Comment 20•11 years ago
|
||
Comment on attachment 8410992 [details] [diff] [review]
patch v2
Review of attachment 8410992 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with the changes suggested in comment 19 included.
Attachment #8410992 -
Flags: review?(mdeboer) → review+
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #8410992 -
Attachment is obsolete: true
Assignee | ||
Comment 22•11 years ago
|
||
the previous patch contained bogus stuff that doesn't belong at all in here
Attachment #8411103 -
Attachment is obsolete: true
Assignee | ||
Comment 23•11 years ago
|
||
... and then I uploaded the wrong file. maybe this is better?
Attachment #8411106 -
Attachment is obsolete: true
Comment 24•11 years ago
|
||
Comment on attachment 8411109 [details] [diff] [review]
patch v3
Verified ;-) Ready for check-in.
Attachment #8411109 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 25•11 years ago
|
||
Keywords: checkin-needed
Comment 26•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Updated•11 years ago
|
Updated•11 years ago
|
Assignee | ||
Comment 27•11 years ago
|
||
I don't intend to uplift this to beta.
Comment 28•11 years ago
|
||
We should relnote/devdoc this for theme impact
Keywords: dev-doc-needed,
relnote
Comment 29•11 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #28)
> We should relnote/devdoc this for theme impact
https://developer.mozilla.org/en-US/Firefox/Releases/31#Changes_for_add-on_and_Mozilla_developers
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 30•11 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #29)
> (In reply to :Gijs Kruitbosch from comment #28)
> > We should relnote/devdoc this for theme impact
>
> https://developer.mozilla.org/en-US/Firefox/Releases/31#Changes_for_add-
> on_and_Mozilla_developers
This seems sufficient; I don't think this belongs in the release notes.
Keywords: relnote
You need to log in
before you can comment on or make changes to this bug.
Description
•