Closed Bug 1196268 Opened 5 years ago Closed 3 years ago

The cancel button is missing the oval button in the Action Menu

Categories

(Firefox OS Graveyard :: Gaia::Components, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.5+, b2g-v2.2 unaffected, b2g-master affected)

RESOLVED WONTFIX
blocking-b2g 2.5+
Tracking Status
b2g-v2.2 --- unaffected
b2g-master --- affected

People

(Reporter: njpark, Assigned: kgrandon)

References

Details

(Keywords: regression, verifyme, Whiteboard: fbimage)

Attachments

(3 files, 1 obsolete file)

STR:
Tap the empty area on homescreen to open the context menu

Expected:
the cancel button should have the oval button

Actual:
There is no such button, just the 'Cancel' caption
(See attached)

Version Info:
Build ID               20150819030215
Gaia Revision          1e1197e0e8e64307aa382ffba4711d1c661de7ca
Gaia Date              2015-08-18 16:54:35
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/f384789a29dcfd514d25d4a16a97ec5309612d78
Gecko Version          43.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150819.063618
Firmware Date          Wed Aug 19 06:36:31 EDT 2015
Bootloader             L1TC000118D0
Keywords: regression
Whiteboard: fbimage
Let's get a window.
[Blocking Requested - why for this release]:
UI regression
blocking-b2g: --- → 2.5?
Issue is not limited to Flame. Saw this on Aries as well. Also it is more global as I saw this when tapping ellipsis icon while on a webpage.
QA Contact: pcheng
Summary: Flame: The cancel button shown while changing wallpaper is missing the oval button → The cancel button shown while changing wallpaper is missing the oval button
mozilla-inbound regression window:

Last Working
Device: Flame
BuildID: 20150817214933
Gaia: d9d99f32762975a370f1abd34a3512bd6fe29111
Gecko: 4b1fccda7a74443b3093efe8d224dad547f0f36b
Version: 43.0a1 (2.5 Master) 
Firmware Version: v18Dv4
User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0

First Broken
Device: Flame
BuildID: 20150817225933
Gaia: d9d99f32762975a370f1abd34a3512bd6fe29111
Gecko: 89a5b5c8ce17eaefdea8178171567ee309bbf447
Version: 43.0a1 (2.5 Master) 
Firmware Version: v18Dv4
User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0

Gaia is the same so it is a Gecko issue.

Gecko pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4b1fccda7a74443b3093efe8d224dad547f0f36b&tochange=89a5b5c8ce17eaefdea8178171567ee309bbf447

Caused by changes made in bug 1179288.
Blocks: 1179288
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmercado)
Robert can you take a look at this?  Seems like the changes for bug 1179288 are the cause of this issue.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmercado) → needinfo?(roc)
[Triage] We should be blocked by this. This bug will be pending on UX suggestion and take further work by developers and QAs.
blocking-b2g: 2.5? → 2.5+
This requires a Gaia change.

Currently the DOM looks like (in devtools):
  <button class="gaia-menu-cancel">
    ::before
    Cancel
  </button>

There is a CSS rule for the <button> that says
  button:last-child {
    position:fixed;
    background:...; /* gray */
    border-radius:2rem;
  }
That's what draws the rounded button background.

There's also a CSS rule for the ::before pseudo-element:
  button:last-child::before {
    position:fixed;
    background:...; /* dark gray */
    z-index:-1;
  }

Before bug 1179288 was fixed, the <button> was not a stacking context since its 'z-index' defaults to 'auto'. So its ::before, being 'z-index:-1', was drawn below the <button>'s own background.

Bug 1179288 brings us into line with what Safari and Chrome have been doing for a couple of years, and what the spec recently changed to say: 'position:fixed' elements always have their own stacking context. So now the ::before pseudoelement's background is drawn above the <button> background.

I don't understand why the Gaia content is written the way it is, so I don't know what change to recommend. Somehow the dark gray background under the button's own background needs to be moved to an element that's not a child of the <button>.
Flags: needinfo?(roc)
FWIW, I didn't like making this incompatible behavior change, but given that Safari and Chrome were deliberately departing from the old CSS spec, refused to change back, and Microsoft followed them, we didn't have much choice.
BTW in case it's not clear, a ::before pseudo-element is always a *child* of the element it belongs to.
Looks like this is a bug in the dialog component that's being used, and it affects more than just the homescreen.

I'd have thought we could just switch the backgrounds around as the minimal change (make the grey background the element background and the red oval the pseudo-element). Pretty certain we can easily work around this though.
Component: Gaia::Homescreen → Gaia::Components
As Chris comments, this issue is affecting to the Action Menu so it can be reproduced in many applications so changing the title according to this
Summary: The cancel button shown while changing wallpaper is missing the oval button → The cancel button is missing the oval button in the Action Menu
Duplicate of this bug: 1202543
Duplicate of this bug: 1203940
I might take this.
Flags: needinfo?(kevingrandon)
Assignee: nobody → kevingrandon
Status: NEW → ASSIGNED
Flags: needinfo?(kevingrandon)
Comment on attachment 8669220 [details] [review]
[gaia] KevinGrandon:bug_1196268_gaia_menu_cancel_outline > mozilla-b2g:master

Simple component change to move around some background colors. R=me.
Attachment #8669220 - Flags: review+
In master: https://github.com/mozilla-b2g/gaia/commit/de66a9ce917bcf606216ac581c3b98bda8750545
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Tested in flame with latest master (10/4) and it seems that has been fixed for the homescreen issue (reported in the description of the bug) but I am still seeing the bug in other Action menus (e.g. in the menu offered when sharing a contact to other apps, when sharing a picture from the Gallery to other apps...) so it seems that the bug has not been resolved completely.

Setting verifyme so QA team can confirm what I am seeing

Environmental Variables for master version (10/4):
flame master (2.5 version)
Build ID: 20151004053334
Gecko: 41e2b2b
Gaia: de66a9c
Platform version: 44.0a1
Keywords: verifyme
A lot of apps (including sms and contacts) are using /shared/js/option_menu.js, which has not been touched here.

I'm reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Julien Wajsberg [:julienw] (away Oct 1st, 2nd) from comment #19)
> A lot of apps (including sms and contacts) are using
> /shared/js/option_menu.js, which has not been touched here.
> 
> I'm reopening.

Thanks Julien. Looks like SMS and Dialer use option_menu.js. I'll try to fix those apps when I have time, though the fix might be in a new bug.
Comment on attachment 8670587 [details] [review]
[gaia] KevinGrandon:bug_1196268_action_menu_button_bg > mozilla-b2g:master

Julien - wondering if you'd be able to put a review stamp on this if it fixes the cases you noticed that were broken.

Alternatively, David if you're free for a review that would work as well. Thanks!
Attachment #8670587 - Flags: review?(felash)
Attachment #8670587 - Flags: review?(dflanagan)
Comment on attachment 8670587 [details] [review]
[gaia] KevinGrandon:bug_1196268_action_menu_button_bg > mozilla-b2g:master

fixes the issue for me but I think we can make it more maintainable, even for a workaround. I left some simple comments on github.

also I wonder why no tests are failing with this change...
Attachment #8670587 - Flags: review?(felash)
Attachment #8670587 - Flags: review?(dflanagan)
Attachment #8670587 - Flags: feedback+
(In reply to Julien Wajsberg [:julienw] (away Oct 1st, 2nd) from comment #23)
> fixes the issue for me but I think we can make it more maintainable, even
> for a workaround. I left some simple comments on github.

Thanks for the review. I agree, I was just trying to take the least riskiest approach, but I can make the requested changes no problem.


> also I wonder why no tests are failing with this change...

Likely because it's not too intrusive, it just adds a wrapping element.
Comment on attachment 8670587 [details] [review]
[gaia] KevinGrandon:bug_1196268_action_menu_button_bg > mozilla-b2g:master

Julien, thank you for the detailed review. Mind giving this one last look?
Attachment #8670587 - Flags: review?(felash)
Comment on attachment 8670587 [details] [review]
[gaia] KevinGrandon:bug_1196268_action_menu_button_bg > mozilla-b2g:master

It works fine in SMS but the activity chooser still doesn't. And actually all the places where we use these dialogs :/ `git grep 'data-type="action"'` in Gaia should show you all locations...

Here is an example STR:
1. open the SMS app, start a "new message".
2. Press the "pick attachment" icon.

=> the "cancel" button look like the other ones.

Another STR in Contacts:
1. open a contact that has a picture.
2. press "edit"
3. tap the existing picture.


And there are dozens of places :/
Attachment #8670587 - Flags: review?(felash)
Sure, and it could very likely be too much work to do all of this in one place. I'll probably turn this into a meta bug and work on individual bugs.
Depends on: 1213135
Attachment #8670587 - Attachment is obsolete: true
Depends on: 1213614
Depends on: 1213615
It wasn't agreed that switching to shared dialog code could be a blocker for 2.5, but here's a situation that is a blocker for 2.5 that would have been much easier to deal with had we done so.

I hope we learn from this that sometimes refactoring, even with no user-visible change (though the increase in consistency would have been user-visible...), should be blocking work.
Not sure you could conclude this. Switching to a shared component would have been more work (though likely more interesting) than adding a container to all related forms.
(In reply to Julien Wajsberg [:julienw] from comment #30)
> Not sure you could conclude this. Switching to a shared component would have
> been more work (though likely more interesting) than adding a container to
> all related forms.

Yes, this one change will be less work than switching over - but if we need to make another change, that comparison becomes less clear. And if we wanted to unify the visuals/motion (which is currently subtly different across a lot of them), that would definitely be a lot more work.

It's just a shame to see so many polish bugs mount up alongside our technical debt with these short-term fixes, when we could be hitting both on the head with a more considered approach :(
Depends on: 1213323
The web component stuff is a _lot_ of work. I've been porting switches over and it's not easy. I would have very much liked to see this go to gaia-menu, but it's just too much work right now. Let's put the quick fix in, then iterate. I'll try to port these use cases over to use gaia menu when I have time.
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][COM=Pin the Web]
QA Whiteboard: [QAnalyst-Triage+][COM=Pin the Web] → [QAnalyst-Triage+]
Depends on: 1202543
Depends on: 1215404
Depends on: 1215371
Kevin, whats the status of this bug?
Flags: needinfo?(kevingrandon)
I took a look and I didn't find any other menus that exhibit this problem. I think this should be fixed, but there's many instances of this menu across the OS. Please verify this.

Moving to web components should help dramatically for issues like this in the future.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Flags: needinfo?(kevingrandon)
Resolution: --- → FIXED
Hi Kevin, cancel button from share ringtone action menu still incorrect. I submitted bug 1222295 to check this issue.
Verification failed I'm able to reproduce Bug 1222295. The oval box is square

Environmental Variables: Bug 1222295 reproed
Device: Flame 2.6
BuildID: 20151109030231
Gaia: c3436122d678911d04b8f491724596116890ff9b
Gecko: e2a910c048dc82fc3be53475f18e7f81f03e377b
Gonk: 205ac4204bbbb2098a8046444acba551ba5dc75a
Version: 45.0a1 (2.6) 
Firmware Version: v18D
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(jmercado)
Let's check this again when bug 1222295 is fixed
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmercado)
Seems many more cases are being found, so re-opening this to use as a meta-bug.

https://bugzilla.mozilla.org/show_bug.cgi?id=1222295#c4
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1223675
Depends on: 1224124
Depends on: 1224510
Depends on: 1225047
Depends on: 1225764
Depends on: 1225765
No longer depends on: 1225764
Depends on: 1230874
See Also: → 1232594
Just found a similar issue in the contacts shared code, I'll open another bug.
Depends on: 1234139
Firefox OS is not being worked on
Status: REOPENED → RESOLVED
Closed: 5 years ago3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.