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

Status: RESOLVED WONTFIX

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

Description

[No-Jun Park [:njpark]]

Attachment: test_homescreen_change_wallpaper_from_gallery_None_flame_en-US_1+2015-08-19-06-47-08.png

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

Comment 1

[Chris Lord [:cwiiis]]

Let's get a window.

Comment 2

[Peter Bylenga [:PBylenga]]

[Blocking Requested - why for this release]:
UI regression

Comment 3

[Pi Wei Cheng [:piwei] (inactive)]

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.

Comment 4

[Pi Wei Cheng [:piwei] (inactive)]

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.

Comment 5

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Robert can you take a look at this?  Seems like the changes for bug 1179288 are the cause of this issue.

Comment 6

[Al Tsai [:atsai]]

[Triage] We should be blocked by this. This bug will be pending on UX suggestion and take further work by developers and QAs.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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>.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

BTW in case it's not clear, a ::before pseudo-element is always a *child* of the element it belongs to.

Comment 10

[Chris Lord [:cwiiis]]

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.

Comment 11

[Maria Angeles Oteo (:oteo)]

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

Comment 14

[Kevin Grandon :kgrandon]

I might take this.

Comment 15

[GitHub Autolander Bot]

Attachment: [gaia] KevinGrandon:bug_1196268_gaia_menu_cancel_outline > mozilla-b2g:master

Comment 16

[Kevin Grandon :kgrandon]

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.

Comment 17

[Kevin Grandon :kgrandon]

In master: https://github.com/mozilla-b2g/gaia/commit/de66a9ce917bcf606216ac581c3b98bda8750545

Comment 18

[Maria Angeles Oteo (:oteo)]

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

Comment 19

[Julien Wajsberg [:julienw]]

A lot of apps (including sms and contacts) are using /shared/js/option_menu.js, which has not been touched here.

I'm reopening.

Comment 20

[Kevin Grandon :kgrandon]

(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 21

[GitHub Autolander Bot]

Attachment: [gaia] KevinGrandon:bug_1196268_action_menu_button_bg > mozilla-b2g:master

Comment 22

[Kevin Grandon :kgrandon]

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!

Comment 23

[Julien Wajsberg [:julienw]]

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...

Comment 24

[Kevin Grandon :kgrandon]

(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 25

[Kevin Grandon :kgrandon]

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?

Comment 26

[Julien Wajsberg [:julienw]]

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 :/

Comment 27

[Kevin Grandon :kgrandon]

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.

Comment 28

[GitHub Autolander Bot]

Attachment: [gaia] KevinGrandon:bug_1213135_shared_action_menu_bg > mozilla-b2g:master

Comment 29

[Chris Lord [:cwiiis]]

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.

Comment 30

[Julien Wajsberg [:julienw]]

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.

Comment 31

[Chris Lord [:cwiiis]]

(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 :(

Comment 32

[Kevin Grandon :kgrandon]

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.

Comment 33

[Gregor Wagner [:gwagner]]

Kevin, whats the status of this bug?

Comment 34

[Kevin Grandon :kgrandon]

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.

Comment 35

[Alison Shiue]

Hi Kevin, cancel button from share ringtone action menu still incorrect. I submitted bug 1222295 to check this issue.

Comment 36

[Max Murrell [MMurrell]]

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

Comment 37

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Let's check this again when bug 1222295 is fixed

Comment 38

[Kevin Grandon :kgrandon]

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

Comment 39

[Gabriele Svelto [:gsvelto] (PTO)]

Just found a similar issue in the contacts shared code, I'll open another bug.

Comment 40

[BMO Automation]

Firefox OS is not being worked on