Closed Bug 997131 Opened 10 years ago Closed 10 years ago

Simplify conditional forward button CSS by making the forward button a direct sibling of the location bar

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 31
Tracking Status
firefox29 --- wontfix
firefox30 --- wontfix
firefox31 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [Australis:P3+][qa-])

Attachments

(5 files, 6 obsolete files)

Attached patch WIP patch (obsolete) — Splinter Review
This works on Linux, haven't tested it elsewhere yet.

Other than simplifying the code, this should pave the way for addressing bug 995300.
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)
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...
Attachment #8407483 - Flags: feedback?(mdeboer)
Attached patch WIP patch 2 (obsolete) — Splinter Review
this should be better on OS X, although it might still have issues
Attachment #8407483 - Attachment is obsolete: true
Attachment #8407676 - Flags: review?(mdeboer)
Blocks: 998157
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?
(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
Whiteboard: [Australis:P5]
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-
(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?
Attached patch patch (obsolete) — Splinter Review
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)
Attached image button-offset-osx.png
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.
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.
(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 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-
Upping prio, because of the bugs it blocks. It's also a shame that we can't uplift this to beta anymore...
Whiteboard: [Australis:P5] → [Australis:P3+]
Attached patch patch v2 (obsolete) — Splinter Review
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)
(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.
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 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+
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #8410992 - Attachment is obsolete: true
Attached patch patch v3 (the real one) (obsolete) — Splinter Review
the previous patch contained bogus stuff that doesn't belong at all in here
Attachment #8411103 - Attachment is obsolete: true
Attached patch patch v3Splinter Review
... and then I uploaded the wrong file. maybe this is better?
Attachment #8411106 - Attachment is obsolete: true
Comment on attachment 8411109 [details] [diff] [review]
patch v3

Verified ;-) Ready for check-in.
Attachment #8411109 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0ff00dde7e0c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
No longer blocks: 1001501
Depends on: 1001501
I don't intend to uplift this to beta.
We should relnote/devdoc this for theme impact
(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
(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
Whiteboard: [Australis:P3+] → [Australis:P3+][qa-]
Depends on: 1320126
You need to log in before you can comment on or make changes to this bug.