Closed Bug 978491 Opened 10 years ago Closed 10 years ago

"Sidebars" widget has no inverted icon in subview on Windows and Linux

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 31
Tracking Status
firefox29 --- unaffected
firefox30 --- fixed
firefox31 --- verified

People

(Reporter: u428464, Assigned: shorlander)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [Australis:P3-])

Attachments

(1 file, 1 obsolete file)

The new sidebars widget has no inverted icon when the subview is shown in the panel. It should for consistency.
Whiteboard: [Australis:P4]
Blocks: 960198
Whiteboard: [Australis:P4] → [Australis:P3-][aurora-unaffected]
Fixed by backing out the sidebars widget ( bug 960198)
Assignee: nobody → jaws
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
The issue is back in latest Nightlies.
Status: RESOLVED → UNCONFIRMED
Resolution: FIXED → ---
The icon is missing from the spritesheet on Windows and Linux. I should have caught this when reviewing 960198 (unless they were removed by one of the other changes...) Shane?
Assignee: jaws → nobody
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mixedpuppy)
Whiteboard: [Australis:P3-][aurora-unaffected] → [Australis:P3-]
I think it may have been caused by bug 969904.
(In reply to Guillaume C. [:ge3k0s] from comment #4)
> I think it may have been caused by bug 969904.

Indeed:

https://hg.mozilla.org/mozilla-central/raw-file/02be46ac2b55/browser/themes/windows/menuPanel-aero.png
https://hg.mozilla.org/mozilla-central/raw-file/80fda301471e/browser/themes/windows/menuPanel-aero.png

Stephen, Mike? I also noticed in this (somewhat 'dumb') visual diff that the find icon was reversed. ISTR that was discussed on IRC and *wasn't* intended? Maybe I misunderstood the conversation at the time...

(note that it seems like the icon was changed to have the sidebar separator further from the right, which means we can't just put back the part of the icon that was deleted. :-( )
Flags: needinfo?(shorlander)
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mdeboer)
Keywords: regression
Summary: "Sidebars" widget has no inverted icon in subview → "Sidebars" widget has no inverted icon in subview on Windows and Linux
Blocks: 969904
No longer blocks: 960198
I didn't catch the missing inverted icon for the sidebars widget. It should be re-added!

The find button should be inverted for windows, compared with OSX, at least that's what Stephen told me. If this is wrong at the moment, then this would mean I checked in the wrong patch and that would make me super sad.
Flags: needinfo?(mdeboer)
Assignee: nobody → shorlander
Status: NEW → ASSIGNED
Flags: needinfo?(shorlander)
(In reply to Mike de Boer [:mikedeboer] from comment #6)
> The find button should be inverted for windows, compared with OSX, at least
> that's what Stephen told me. If this is wrong at the moment, then this would
> mean I checked in the wrong patch and that would make me super sad.

As much as I hate being the bearer of bad news:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/menuPanel.png
http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/menuPanel.png

have it pointing the same direction.
Yes, it seemed like Stephen didn't update the menuPanel.png resource properly and I didn't check thoroughly enough, which I should've done.

Stephen, can you post a corrected menuPanel.png in this bug please? I can produce/ review the patch if you'd like. Thanks!
Flags: needinfo?(shorlander)
Attachment #8393702 - Flags: review?(mdeboer)
Flags: needinfo?(shorlander)
Attachment #8393702 - Flags: review?(mdeboer) → review+
If the find icon is inverted in menu panel, it'd be nice if the toolbar icon was too (for consistency). It's not currently the case.
(In reply to Tim Nguyen [:ntim] from comment #10)
> If the find icon is inverted in menu panel, it'd be nice if the toolbar icon
> was too (for consistency). It's not currently the case.

Yes, would be nice for the toolbar also. I don't think it currently has a toggled state attribute though.
https://hg.mozilla.org/mozilla-central/rev/970e6edabe39
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Is it on Aurora ?
Comment on attachment 8393702 [details] [diff] [review]
add-inverted-sidebars-icon.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 969904
User impact if declined: No inverted icon for sidebar's subview
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky):  Low, small image updating
String or IDL/UUID changes made by this patch: None
Attachment #8393702 - Flags: approval-mozilla-aurora?
Tim, please leave requesting uplift to the patch author or reviewer. Not all patches can be uplifted without changes and it is better for the author/reviewer to coordinate uplift.
Attachment #8393702 - Flags: approval-mozilla-aurora?
Comment on attachment 8393702 [details] [diff] [review]
add-inverted-sidebars-icon.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 969904
User impact if declined: No inverted icon for sidebar's subview
Testing completed (on m-c, etc.): landed on m-c, looking a-ok!
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: n/a
Attachment #8393702 - Flags: approval-mozilla-beta?
Attachment #8393702 - Flags: approval-mozilla-aurora?
Attachment #8393702 - Flags: approval-mozilla-beta?
Attachment #8393702 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attached patch Patch for linux (obsolete) — Splinter Review
Shorlander forgot to fix the problem for linux.
Here's a patch that uses the Windows menuPanel.png asset for linux (as it always did before).
Attachment #8395939 - Flags: review?(gijskruitbosch+bugs)
Attachment #8395939 - Flags: ui-review?(shorlander)
Comment on attachment 8395939 [details] [diff] [review]
Patch for linux

Clearing and filing a new bug per IRC.
Attachment #8395939 - Flags: ui-review?(shorlander)
Attachment #8395939 - Flags: review?(gijskruitbosch+bugs)
Attachment #8395939 - Attachment is obsolete: true
Depends on: 987388
Flags: in-testsuite?
Keywords: verifyme
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0

Tested the fix on Windows 7 64bit using Firefox 31 beta 5, build ID: 987388

The Linux fix for this issue was verified in bug 987388.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: