Closed
Bug 1011604
Opened 10 years ago
Closed 10 years ago
[Messages] Update to use <gaia-header>
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
Tracking
(ux-b2g:2.1, b2g-v2.1 fixed)
Tracking | Status | |
---|---|---|
b2g-v2.1 | --- | fixed |
People
(Reporter: yor, Assigned: wilsonpage)
References
Details
Attachments
(1 file, 1 obsolete file)
No description provided.
Summary: [SMS] Update to use <gaia-header> → [Messages] Update to use <gaia-header>
Updated•10 years ago
|
Blocks: gaia-header
Assignee | ||
Updated•10 years ago
|
Assignee: yor → wilsonpage
Comment 1•10 years ago
|
||
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
Attachment #8476214 -
Flags: review?(felash) → review?(schung)
Comment 3•10 years ago
|
||
Comment on attachment 8476214 [details]
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)
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Steve Chung [:steveck] from comment #3) > Comment on attachment 8476214 [details] > 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•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
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•10 years ago
|
||
(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.
Assignee | ||
Comment 8•10 years ago
|
||
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)
Comment 9•10 years ago
|
||
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)
Comment 10•10 years ago
|
||
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)
Comment 11•10 years ago
|
||
(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)
Reporter | ||
Comment 12•10 years ago
|
||
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•10 years ago
|
||
Comment on attachment 8476214 [details]
PR
Added few more comments on GitHub.
Please, set r? once you're ready again.
Thanks!
Attachment #8476214 -
Flags: review?(azasypkin)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8476214 -
Attachment is obsolete: true
Attachment #8477386 -
Flags: review?(drs+bugzilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8477386 -
Flags: review?(drs+bugzilla) → review?(azasypkin)
Assignee | ||
Updated•10 years ago
|
Assignee: yor → wilsonpage
Comment 15•10 years ago
|
||
Comment on attachment 8477386 [details]
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 16•10 years ago
|
||
Comment on attachment 8477386 [details] 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 17•10 years ago
|
||
Comment on attachment 8477386 [details] 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)
Comment 18•10 years ago
|
||
(In reply to Steve Chung [:steveck] from comment #17) > Comment on attachment 8477386 [details] > 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.
Keywords: checkin-needed
Comment 20•10 years ago
|
||
Master: https://github.com/mozilla-b2g/gaia/commit/a5cb0cc3515474040036f41f55f14f2d90ef6da8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-b2g-v2.1:
--- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S3 (29aug)
You need to log in
before you can comment on or make changes to this bug.
Description
•