Closed Bug 992637 Opened 10 years ago Closed 10 years ago

Remove the background color from subview headers and make them all-caps instead such that the background matches the panel arrow color

Categories

(Firefox :: Theme, defect)

30 Branch
defect
Not set
normal
Points:
1

Tracking

()

VERIFIED FIXED
Firefox 35
Iteration:
35.3

People

(Reporter: micmon, Assigned: dao)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P-])

Attachments

(3 files, 1 obsolete file)

Open the help or developer menu for example. The new menu will slide in from the right side and have a darker header than before. The arrow above the whole menu is still using the old (lighter) color).
Whiteboard: dupeme
The arrow is an image, and so can't really adapt to varying panel colors. We'd need to use <canvas> or SVG and dynamically update it. (This has been an issue for Social API panels and other places, where the panel content is essentially an <iframe> and can have anything in it.)
Whiteboard: dupeme → dupeme [Australis:P-]
Attached patch menupanel-arrow.diff (obsolete) — Splinter Review
like bug 1057030
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #8495482 - Flags: review?(jaws)
Iteration: --- → 35.2
Points: --- → 1
Flags: qe-verify+
Flags: firefox-backlog+
OS: Linux → All
Hardware: x86_64 → All
Comment on attachment 8495482 [details] [diff] [review]
menupanel-arrow.diff

Review of attachment 8495482 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure if we want to use a gradient here. Much of this design was to use flat colors and we don't have many gradients in the menu.

Philipp/Stephen, are you OK with this change? Or should change the color of these headers to match the arrow and thus change the color of the body area to be different?
Attachment #8495482 - Flags: ui-review?(shorlander)
Attachment #8495482 - Flags: ui-review?(philipp)
I had meant to include a screenshot of it for Philipp or Stephen to get their response quicker. Here it is, http://screencast.com/t/dVZNVHhVnw7o
The current color mismatch is obviously unintended and somewhat jarring. Can we agree that the gradient is an improvement over that and get this landed? Changing the subview colors more broadly can be investigated in a different bug.
It has existed since v29, so I don't see the urgency in fixing it immediately before we get a better design. Ideally we would be able to get the color of the arrow to match the grey instead of fading everything in to white.
I didn't say it's urgent, it's just a bug that we should fix while not ruling out that there might be a better fix down the road. That this bug existed for so long is a little bit embarrassing and not a good reason to wait even longer.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> Ideally we would be able to get
> the color of the arrow to match the grey instead of fading everything in to
> white.

I disagree that we should start shipping custom arrow images on a per-panel basis. That would be a pretty significant maintenance burden that can be avoided easily when designing stuff, by taking into account that we have a standard panel background that's expected to connect with the arrow.

In this particular case changing arrow would be even more dubious since the main view and the sub view aren't consistent with what backgrounds the put next to the arrow, so I guess the arrow image would need to be swapped dynamically.
If we switched to using an SVG arrow then the color of the arrow could be changed dynamically.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> If we switched to using an SVG arrow then the color of the arrow could be
> changed dynamically.

How so?
(In reply to Dão Gottwald [:dao] from comment #9)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> > If we switched to using an SVG arrow then the color of the arrow could be
> > changed dynamically.
> 
> How so?

Similar to how we did http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/content-contextmenu.svg?raw=1 with http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/contextmenu.inc.css
We'd still need to fork it to browser. Anyway, that's significantly more work than what my patch does and since (like you argued) this bug isn't a priority, I don't see us spending time on this anytime soon, and therefore I'd like to land my stop-gap solution.
Iteration: 35.2 → 35.3
Comment on attachment 8495482 [details] [diff] [review]
menupanel-arrow.diff

> <MattN> jaws: Is there a way to change the doorhanger arrow color? i.e. how is that color specified?
> <MattN> ah, nvm, I see it's an image
> <MattN> I somehow overlooked .panel-arrowbox
> <jaws>  i would prefer that we went the SVG route, since we are now using SVG in other places of the browser
> <shorlander>  jaws: I agree with you. It's not pressing and I would rather wait for the right design than do that gradient
> <shorlander>  I had some ideas, and I know mmaslaney did some design work around that
> <shorlander>  We talked about just removing the grey area anyway since it was too similar to the footer button.
> <shorlander>  jaws, MattN: Are there really any problems going the SVG route? Just switch out the graphic if panel has a header attribute?
> <MattN> just not a priority probably
> <jaws>  shorlander: dao says that the code would need to be forked to browser. i'm not sure why, as other products may want the themeing ability
> <MattN> I agree
> <MattN> not sure why
Attachment #8495482 - Flags: ui-review?(shorlander)
Attachment #8495482 - Flags: ui-review?(philipp)
Attachment #8495482 - Flags: ui-review-
Attachment #8495482 - Flags: review?(jaws)
Comment on attachment 8495482 [details] [diff] [review]
menupanel-arrow.diff

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> > <jaws>  shorlander: dao says that the code would need to be forked to browser. i'm not sure why, as other products may want the themeing ability

It's not about the ability, it's about the particular color this particular panel wants.

If you read the previous comments, you'll see that I wasn't presenting the gradient as the ideal solution. It's the simplest solution and it's better than the color mismatch. I can't justify spending more time on a more complicated solution right now, so we either take this patch, or someone else takes this bug, or it goes back to zombie mode and we keep shipping the obvious color mismatch.
Attachment #8495482 - Flags: ui-review- → ui-review?(shorlander)
The other option that shorlander mentioned in comment #12 was that we could look in to removing the header color altogether. "We talked about just removing the grey area anyway since it was too similar to the footer button."

Stephen, if we went that route, would we need to make any other color changes to the rest of the panel?
QA Contact: cornel.ionce
Comment on attachment 8495482 [details] [diff] [review]
menupanel-arrow.diff

Review of attachment 8495482 [details] [diff] [review]:
-----------------------------------------------------------------

I would rather not introduce a gradient here.

We did talk about this issue in post Australis follow-up. The solution that came up was to remove the background-color and give the title an all caps treatment. Solves this problem and removes any confusion with the actionable footer.

http://cl.ly/image/20341S04420T

The other alternative would be to change the color of the arrow based on whether or not a subview is open.
Attachment #8495482 - Flags: ui-review?(shorlander) → ui-review-
Attached patch uppercase.diffSplinter Review
Attachment #8495482 - Attachment is obsolete: true
Attachment #8503003 - Flags: review?(jaws)
Comment on attachment 8503003 [details] [diff] [review]
uppercase.diff

Review of attachment 8503003 [details] [diff] [review]:
-----------------------------------------------------------------

Tested the patch out, looks good.
Attachment #8503003 - Flags: review?(jaws) → review+
https://hg.mozilla.org/integration/fx-team/rev/08261d2edbed
Summary: Color of arrow in slide menus → Remove the background color from subview headers and make them all-caps instead such that the background matches the panel arrow color
Whiteboard: dupeme [Australis:P-] → [Australis:P-]
https://hg.mozilla.org/mozilla-central/rev/08261d2edbed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Comment on attachment 8503003 [details] [diff] [review]
uppercase.diff

Approval Request Comment
[Feature/regressing bug #]: australis
[User impact if declined]: graphical glitch
[Describe test coverage new/current, TBPL]: no test coverage
[Risks and why]: pretty low, small CSS patch
[String/UUID change made/needed]: none
Attachment #8503003 - Flags: approval-mozilla-aurora?
pike/flod - Does transforming the menu title to all caps have any impact on l10n?
Flags: needinfo?(l10n)
Flags: needinfo?(francesco.lodolo)
(In reply to Lawrence Mandel [:lmandel] from comment #22)
> pike/flod - Does transforming the menu title to all caps have any impact on
> l10n?

Yes, it's not safe. See bug 830002 for a nice read.
Flags: needinfo?(francesco.lodolo)
(In reply to Francesco Lodolo [:flod] from comment #23)
> (In reply to Lawrence Mandel [:lmandel] from comment #22)
> > pike/flod - Does transforming the menu title to all caps have any impact on
> > l10n?
> 
> Yes, it's not safe. See bug 830002 for a nice read.

So there was an issue with Greek which should be fixed and there's an issue with Irish where the first letter may be lowercase to express a semantic difference. Seems like we could avoid the latter issue by using font-variant: small-caps instead of text-transform: uppercase?
I think it works for most locales, but not all of them (exactly like uppercase transformation)
https://groups.google.com/forum/#!msg/mozilla.dev.l10n.web/UDjtZgvH3Lg/O6T906KaJTQJ

> The translation in the PO file has the word "t-eolas" in lowercase, but
> on the button it looks like "T-EOLAS" which is goofy looking - these prefix
> letters are never capitalized in Irish; it should look like "tEolas"
> or even "tEOLAS"
> (note no hyphen in either case with an uppercase "E").
(In reply to Francesco Lodolo [:flod] from comment #25)
> I think it works for most locales, but not all of them (exactly like
> uppercase transformation)
> https://groups.google.com/forum/#!msg/mozilla.dev.l10n.web/UDjtZgvH3Lg/
> O6T906KaJTQJ
> 
> > The translation in the PO file has the word "t-eolas" in lowercase, but
> > on the button it looks like "T-EOLAS" which is goofy looking - these prefix
> > letters are never capitalized in Irish; it should look like "tEolas"
> > or even "tEOLAS"
> > (note no hyphen in either case with an uppercase "E").

I specifically referred to Irish in my previous comment. As I understand it, small-caps should preserve the distinction between the lowercase t and the uppercase E: ᴛEᴏʟᴀs / ᴛEOLAS
It's an issue if the original word has an hyphen, and the uppercase/small-caps version doesn't need one.
Again, we're probably talking about edge cases, we might also want to NI Kevin to shed some light on this.
(In reply to Francesco Lodolo [:flod] from comment #27)
> It's an issue if the original word has an hyphen, and the
> uppercase/small-caps version doesn't need one.

font-variant: small-caps would transform t-eolas to ᴛ-ᴇᴏʟᴀs, i.e. all caps would be small. Technically the word doesn't become capitalized, it's just a different stylistic representation of the lowercase word.
Attachment #8503560 - Flags: review?(jaws)
Attachment #8503003 - Flags: approval-mozilla-aurora?
Flags: needinfo?(l10n)
Reopening as it looks like we need a further change or a backout on m-c.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8503003 - Flags: checkin+
Comment on attachment 8503560 [details] [diff] [review]
followup, use small-caps

Who wants to review this over the weekend?
Attachment #8503560 - Flags: review?(mconley)
Attachment #8503560 - Flags: review?(gijskruitbosch+bugs)
Attachment #8503560 - Flags: review?(mconley)
Attachment #8503560 - Flags: review?(jaws)
Attachment #8503560 - Flags: review?(gijskruitbosch+bugs)
Attachment #8503560 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1ceec2777843
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
The background color of the subviews header is removed from latest Nightly, build ID: 20141012030203.

Tested on Windows 7 64-bit, Mac OS X 10.8.5 and Ubuntu 14.04 32-bit.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.