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)

ARM
Gonk (Firefox OS)
defect
Not set
normal

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.
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.
Keywords: regression
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
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.
Can you include a screenshot? What's the impact if we don't fix this?
Flags: needinfo?(gsvelto)
Attached image 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.
Flags: needinfo?(gsvelto)
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)
(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?
This is a future certification blocker, please fix it in 2.0.
blocking-b2g: 2.0? → 2.0+
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+
(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.
(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" :)
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Whiteboard: [2.0-flame-test-run-2]
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
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 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+
(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.
(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?
(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.
Updated & refreshed patch, carrying over the r+ flag.
Attachment #8452173 - Flags: review+
Attachment #8450869 - Attachment is obsolete: true
Pushed to gaia/master 9634534545d4c17f7265029191a32afc662fcca0

https://github.com/mozilla-b2g/gaia/commit/9634534545d4c17f7265029191a32afc662fcca0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: