Closed
Bug 1014706
Opened 10 years ago
Closed 10 years ago
The "Accept" button is visible on the SI/SL messages UI
Categories
(Firefox OS Graveyard :: Gaia::Wappush, defect)
Tracking
(blocking-b2g:2.0+, b2g-v1.3 unaffected, b2g-v1.3T unaffected, b2g-v1.4 unaffected, b2g-v2.0 fixed, b2g-v2.1 fixed)
RESOLVED
FIXED
blocking-b2g | 2.0+ |
Tracking | Status | |
---|---|---|
b2g-v1.3 | --- | unaffected |
b2g-v1.3T | --- | unaffected |
b2g-v1.4 | --- | unaffected |
b2g-v2.0 | --- | fixed |
b2g-v2.1 | --- | fixed |
People
(Reporter: gsvelto, Assigned: gsvelto)
References
Details
(Keywords: regression, Whiteboard: [2.0-flame-test-run-2])
Attachments
(3 files, 3 obsolete files)
The "Accept" button used to accept content provisioning WAP Push messages is visible also in the UI used for SI/SL messages. This looks like a regression as it happens on master but not on v1.4 but I couldn't find out what change actually caused it.
Assignee | ||
Comment 1•10 years ago
|
||
This was definitely caused by one of the visual refresh bugs though I couldn't identify which one precisely. It's a CSS change that sets "display: block" on all header buttons and thus causes the "Accept" button to become visible even though it's got the "hidden" attribute.
Assignee | ||
Comment 2•10 years ago
|
||
This patch applies on top of attachment 8427136 [details] [diff] [review] and fixes this issue using the same style used by the Music app to hide the toolbar buttons. I have left the 'hidden' attribute in the accept button because while it's not needed anymore for visibility it might be useful for screen reading.
Comment 3•10 years ago
|
||
Can you include a screenshot? What's the impact if we don't fix this?
Flags: needinfo?(gsvelto)
Assignee | ||
Comment 4•10 years ago
|
||
Here's a screenshot of the issue. I would qualify the impact on the user as confusing: the button is there and you can tap it; if you do we'll open an overlay saying that the configuration could not be applied. This stems from the fact that the "Accept" button is used for content provisioning messages that always contain some configuration information. This is harmless but definitely confusing.
Flags: needinfo?(gsvelto)
Assignee | ||
Comment 5•10 years ago
|
||
This patch fixes the issues and provides additional test coverage. It applies on top of attachment 8430039 [details] [diff] [review].
Assignee: nobody → gsvelto
Attachment #8427841 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8430045 -
Flags: review?(felash)
Comment 7•10 years ago
|
||
(In reply to Gabriele Svelto [:gsvelto] from comment #4) > Created attachment 8429296 [details] > Screenshot of the issue > > Here's a screenshot of the issue. I would qualify the impact on the user as > confusing: the button is there and you can tap it; if you do we'll open an > overlay saying that the configuration could not be applied. This stems from > the fact that the "Accept" button is used for content provisioning messages > that always contain some configuration information. This is harmless but > definitely confusing. The dupe of this bug definitely proves this exposed broken confusing UX.
blocking-b2g: --- → 2.0?
Comment 8•10 years ago
|
||
This is a future certification blocker, please fix it in 2.0.
Updated•10 years ago
|
blocking-b2g: 2.0? → 2.0+
Comment 9•10 years ago
|
||
Comment on attachment 8430045 [details] [diff] [review] [PATCH v2] Hide the accept button when in SI/SL mode Review of attachment 8430045 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the nit about "hidden". If you decide to simplify the class handling (but maybe you have some future code in mind), please request another review. ::: apps/wappush/index.html @@ +43,5 @@ > <section role="region"> > <header> > <button id="close"><span class="icon icon-close" data-l10n-id="close"> close </span></button> > <menu type="toolbar"> > <button id="accept" data-l10n-id="accept" class="full recommend" hidden>Accept</button> we should remove "hidden" if it doesn't work anymore ::: apps/wappush/js/wappush.js @@ +169,5 @@ > + document.body.classList.remove(targetClass); > + }); > + > + document.body.classList.add(mode); > + } seems somewhat overkill for only 2 modes... How about having "display: none" by default, and only adding or removing the class "cp-mode" ?
Attachment #8430045 -
Flags: review?(felash) → review+
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Julien Wajsberg [:julienw] (away until 2nd June) from comment #9) > If you decide to simplify the class handling (but maybe you have some future > code in mind), please request another review. Actually we might want to wait for bug 1015300. The header is being replaced there and the proposed patch does fix this problem too w/o having to add this hack. The small issue is that the proposed patch does hide the Accept button correctly but messes up everything else :-P > we should remove "hidden" if it doesn't work anymore I thought about doing that but was unsure if keeping it might have been important for an on-screen reader. > seems somewhat overkill for only 2 modes... How about having "display: none" > by default, and only adding or removing the class "cp-mode" ? Yes, I could do that too. I'll wait to see what happens in bug 1015300 and if it doesn't fix the issue here I'll simplify the patch as per your comments and ask for review again.
Comment 11•10 years ago
|
||
(In reply to Gabriele Svelto [:gsvelto] from comment #10) > > we should remove "hidden" if it doesn't work anymore > > I thought about doing that but was unsure if keeping it might have been > important for an on-screen reader. Fortunately the reader knows about "display: none" :)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Whiteboard: [2.0-flame-test-run-2]
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
Assignee | ||
Comment 12•10 years ago
|
||
There's been no activity in bug 1015300 and we need this for 2.0 so here's a more straightforward fix following your advice in the previous review.
Attachment #8430045 -
Attachment is obsolete: true
Attachment #8450869 -
Flags: review?(felash)
Comment 13•10 years ago
|
||
Comment on attachment 8450869 [details] [diff] [review] [PATCH v3] Hide the accept button when in SI/SL mode Review of attachment 8450869 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the nits in the tests ::: apps/wappush/js/wappush.js @@ +154,5 @@ > + * > + * @param {Boolean} enabled Shows the accept button when true, hides it > + * otherwise. > + */ > + function wpm_enableAcceptButton(enabled) { I don't like such boolean parameters usually, but here I think it's readable enough with the function name. I don't like "enable" either, because it implies that the button is displayed but disabled/enabled; instead of displayed/hidden. That said, I don't want that we change the name now, because there is always a high risk of forgetting a rename and break everything :) So let's keep it like this, it's good enough. ::: apps/wappush/test/unit/wappush_test.js @@ +176,5 @@ > title = document.getElementById('title'); > screen = document.getElementById('si-sl-screen'); > container = screen.querySelector('.container'); > text = container.querySelector('p'); > link = container.querySelector('a'); I don't get why you don't declare the variable here instead of doing it in the suite's body? @@ +258,5 @@ > title = document.getElementById('title'); > screen = document.getElementById('si-sl-screen'); > container = screen.querySelector('.container'); > text = container.querySelector('p'); > link = container.querySelector('a'); same here
Attachment #8450869 -
Flags: review?(felash) → review+
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #13) > I don't like "enable" either, because it implies that the button is > displayed but disabled/enabled; instead of displayed/hidden. That said, I > don't want that we change the name now, because there is always a high risk > of forgetting a rename and break everything :) So let's keep it like this, > it's good enough. Good point. This code should go away soon enough when we get the new web components headers. > I don't get why you don't declare the variable here instead of doing it in > the suite's body? Because I load the HTML body as part of the general setup() method; I decided to do so to get a fresh DOM for every test run. I could move it to a suiteSetup() function and the getElementById() calls too if you feel like it would be cleaner.
Comment 15•10 years ago
|
||
(In reply to Gabriele Svelto [:gsvelto] from comment #14) > (In reply to Julien Wajsberg [:julienw] from comment #13) > > I don't like "enable" either, because it implies that the button is > > displayed but disabled/enabled; instead of displayed/hidden. That said, I > > don't want that we change the name now, because there is always a high risk > > of forgetting a rename and break everything :) So let's keep it like this, > > it's good enough. > > Good point. This code should go away soon enough when we get the new web > components headers. > > > I don't get why you don't declare the variable here instead of doing it in > > the suite's body? > > Because I load the HTML body as part of the general setup() method; I > decided to do so to get a fresh DOM for every test run. I could move it to a > suiteSetup() function and the getElementById() calls too if you feel like it > would be cleaner. No no, that's fine to get a fresh DOM every test run, but why don't you declare the node variable using "var" inside your test?
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #15) > No no, that's fine to get a fresh DOM every test run, but why don't you > declare the node variable using "var" inside your test? Good point, no particular reason :) I'm rebasing and refreshing the patch with this change.
Assignee | ||
Comment 17•10 years ago
|
||
Updated & refreshed patch, carrying over the r+ flag.
Attachment #8452173 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8450869 -
Attachment is obsolete: true
Assignee | ||
Comment 18•10 years ago
|
||
Try runs are here: https://travis-ci.org/mozilla-b2g/gaia/builds/29394016 https://tbpl.mozilla.org/?rev=2a965de957dc1891b7c7196480fa2a874c7c4cce&tree=Gaia-Try
Assignee | ||
Comment 19•10 years ago
|
||
Pushed to gaia/master 9634534545d4c17f7265029191a32afc662fcca0 https://github.com/mozilla-b2g/gaia/commit/9634534545d4c17f7265029191a32afc662fcca0
Assignee | ||
Comment 20•10 years ago
|
||
Pushed to gaia/v2.0 503a93bf00780c230325e31d0320a9f282d40f85 https://github.com/mozilla-b2g/gaia/commit/503a93bf00780c230325e31d0320a9f282d40f85
You need to log in
before you can comment on or make changes to this bug.
Description
•