Closed Bug 1011604 Opened 6 years ago Closed 6 years ago

[Messages] Update to use <gaia-header>

Categories

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

x86
macOS
defect
Not set

Tracking

(ux-b2g:2.1, b2g-v2.1 fixed)

RESOLVED FIXED
2.1 S3 (29aug)
ux-b2g 2.1
Tracking Status
b2g-v2.1 --- fixed

People

(Reporter: yor, Assigned: wilsonpage)

References

Details

Attachments

(1 file, 1 obsolete file)

46 bytes, text/x-github-pull-request
azasypkin
: review+
Details | Review
No description provided.
Summary: [SMS] Update to use <gaia-header> → [Messages] Update to use <gaia-header>
Assignee: nobody → yor
Status: NEW → ASSIGNED
Status: ASSIGNED → NEW
Assignee: yor → wilsonpage
Marking this bug for 2.1 since the web components Header is the one committed web component for 2.1. Other web components for 2.2 depend on Header going first. 

I know, however, that the SMS (and email) app is more complicated in terms of implementing the header, which means that this may not land until 2.2, which is OK. This particular bug should not block 2.1.

Sorry for doing this late; Hema just brought this bug to my attention today, but it reflects agreed scope and is in the agreed 2.1 plan. Just updating the flags appropriately for release tracking.
ux-b2g: --- → 2.1
Assignee: wilsonpage → yor
Status: NEW → ASSIGNED
Attached file PR (obsolete) —
Attachment #8476214 - Flags: review?(felash)
Attachment #8476214 - Flags: review?(felash) → review?(schung)
Comment on attachment 8476214 [details] [review]
PR

Redirect review to Oleg since I'll take PTO tomorrow.

Hi Oleg, could you please have a look about the new gaia header web component? I think we need to beware of the complicate back/close button changes and try to avoid unnecessary use of !important in css, thanks!
Attachment #8476214 - Flags: review?(schung) → review?(azasypkin)
(In reply to Steve Chung [:steveck] from comment #3)
> Comment on attachment 8476214 [details] [review]
> PR
> 
> Redirect review to Oleg since I'll take PTO tomorrow.
> 
> Hi Oleg, could you please have a look about the new gaia header web
> component? I think we need to beware of the complicate back/close button
> changes and try to avoid unnecessary use of !important in css, thanks!

Unfortunately !important is required in places as we have no other way to override the specificity of <style scoped>. This is a short-term fix until we get ::content CSS selector in the platform.
Hey Yura,

We need you advise here :)

In this bug we're trying to update SMS app with new gaia-header web component, but it seems that screen reader doesn't recognize back/close buttons inside it. So part of navigation inside app is broken.

So the question is whether it's acceptable regression for v2.1 or not?

Wilson, could you please share more details on the reasons and possible solutions.

Thanks!
Flags: needinfo?(yzenevich)
Flags: needinfo?(wilsonpage)
SMS is one of the core apps that needs to be accessible for 2.1, our first end-user release for accessibility. So no, having no Back and Close buttons in there is not acceptable. So what needs to happen is that the code be checked, and if we have a bug in the platform, fix it, and/or the markup of the web component, or not use it if the problem can't be solved. We're already fighting a battle with the status bar magic.
(In reply to Marco Zehe (:MarcoZ) from comment #6)
> SMS is one of the core apps that needs to be accessible for 2.1, our first
> end-user release for accessibility. So no, having no Back and Close buttons
> in there is not acceptable. So what needs to happen is that the code be
> checked, and if we have a bug in the platform, fix it, and/or the markup of
> the web component, or not use it if the problem can't be solved. We're
> already fighting a battle with the status bar magic.

Thanks Marco! So let's wait for Wilson reply to see what we can do here.
azasypkin, MarcoZ: The back button is encapsulated in the shadow-dom, so this is likely why it's having problems being recognised. As a short term fix we can ignore the 'action' feature of <gaia-header> and use a 'real' button instead, similar to how the previous building-block worked.

I have the following questions:

- Who implements the screen-reader functionality?
- Why doesn't screen-reader detect elements in the shadow-dom?
- Is this a scheduled feature?
Flags: needinfo?(wilsonpage) → needinfo?(marco.zehe)
Redirecting this question to Eitan since he has been doing the most work with kgrandon on Gaia web components. To my knowledge it *should* already be recognized, but I currently lack the in-depth knowledge to confirm that. Eitan, can you jump in, please?
Flags: needinfo?(marco.zehe) → needinfo?(eitan)
I need to test this, but I suspect the issue is that the button is not explorable by touch, but is available through swiping left and right.

If that is indeed the case, the issue, i believe, will only be remedied once bug 887541 lands.
Flags: needinfo?(eitan)
(In reply to Wilson Page [:wilsonpage] from comment #8)
> azasypkin, MarcoZ: The back button is encapsulated in the shadow-dom, so
> this is likely why it's having problems being recognised. As a short term
> fix we can ignore the 'action' feature of <gaia-header> and use a 'real'
> button instead, similar to how the previous building-block worked.
> 
> I have the following questions:
> 
> - Who implements the screen-reader functionality?
Eitan, Marco and myself.
> - Why doesn't screen-reader detect elements in the shadow-dom?
Accessibility module has access to shadow DOM. The problem here is that pointer events do not get to the actual button element and are captured by the parent. (The symptoms Eitan is describing are exactly what's happening).
> - Is this a scheduled feature?
The issue above is tracked in bug 887541 (as already mentioned). Making it a blocker for this one to get things going.
Flags: needinfo?(yzenevich)
I have not been able to test with SMS app but with the Keyboard app which is already using gaia-header, the back button it not available by touch but is through swiping.

So based on Eitan's comments above, bug 887541 seems to be a blocker for this patch.
Comment on attachment 8476214 [details] [review]
PR

Added few more comments on GitHub.

Please, set r? once you're ready again.

Thanks!
Attachment #8476214 - Flags: review?(azasypkin)
Attached file pull-request (master)
Attachment #8476214 - Attachment is obsolete: true
Attachment #8477386 - Flags: review?(drs+bugzilla)
Attachment #8477386 - Flags: review?(drs+bugzilla) → review?(azasypkin)
Assignee: yor → wilsonpage
Comment on attachment 8477386 [details] [review]
pull-request (master)

It works as expected now, I've just left one proposition on Github.

Thanks for you patience!
Attachment #8477386 - Flags: review?(azasypkin)
Comment on attachment 8477386 [details] [review]
pull-request (master)

Ok, looks good to me now, just left two tiny nits on Github! 

Please also wait for feedback from Steve and rebase patch on master once patch for bug 1057996 is landed to be sure that all tests are passing (I'll land it as soon as gaia tree is open).

And let's hope that datazilla won't show any performance regressions :)

Thanks!
Attachment #8477386 - Flags: review+
Attachment #8477386 - Flags: feedback?(schung)
Comment on attachment 8477386 [details] [review]
pull-request (master)

Patch looks good, except a regression I discovered. Please see my comment in https://github.com/mozilla-b2g/gaia/pull/23203/files#r16825931. You could fix the script in this patch, but I'm fine that creating a follow up issue to address the script part later and keep the original header styling override.
Attachment #8477386 - Flags: feedback?(schung)
(In reply to Steve Chung [:steveck] from comment #17)
> Comment on attachment 8477386 [details] [review]
> pull-request (master)
> 
> Patch looks good, except a regression I discovered. Please see my comment in
> https://github.com/mozilla-b2g/gaia/pull/23203/files#﷒0﷓. You could fix the
> script in this patch, but I'm fine that creating a follow up issue to
> address the script part later and keep the original header styling override.

The reproduce step: Entering a tread and the contacts in header is valid with photo. Tap on the header and it will display an option menu. Check the header in menu and you could see the header height is too narrow to display all the information.
This is green and ready to land as soon as Gaia is open again.
Master: https://github.com/mozilla-b2g/gaia/commit/a5cb0cc3515474040036f41f55f14f2d90ef6da8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S3 (29aug)
Depends on: 1060661
Duplicate of this bug: 1024608
You need to log in before you can comment on or make changes to this bug.