1011604 - [Messages] Update to use <gaia-header>

Status: RESOLVED FIXED

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

Comment 1

[Stephany Wilkes]

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.

Comment 2

[Yan Or]

Attachment: PR

Comment 3

[Steve Chung [:steveck]]

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!

Comment 4

[Wilson Page [:wilsonpage]]

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

Comment 5

[Oleg Zasypkin [:azasypkin]]

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!

Comment 6

[Marco Zehe (:MarcoZ)]

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.

Comment 7

[Oleg Zasypkin [:azasypkin]]

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

Comment 8

[Wilson Page [:wilsonpage]]

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?

Comment 9

[Marco Zehe (:MarcoZ)]

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?

Comment 10

[Eitan Isaacson [:eeejay]]

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.

Comment 11

[Yura Zenevich [:yzen] (please use "needinfo")]

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

Comment 12

[Yan Or]

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 13

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8476214 [details] [review]
PR

Added few more comments on GitHub.

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

Thanks!

Comment 14

[Wilson Page [:wilsonpage]]

Attachment: pull-request (master)

Comment 15

[Oleg Zasypkin [:azasypkin]]

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!

Comment 16

[Oleg Zasypkin [: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!

Comment 17

[Steve Chung [:steveck]]

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.

Comment 18

[Steve Chung [:steveck]]

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

Comment 19

[Wilson Page [:wilsonpage]]

This is green and ready to land as soon as Gaia is open again.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/a5cb0cc3515474040036f41f55f14f2d90ef6da8