Closed
Bug 921928
Opened 11 years ago
Closed 11 years ago
[System][UX] The dialer layout is hard to use while make a second phone call
Categories
(Firefox OS Graveyard :: Gaia::Dialer, defect)
Tracking
(blocking-b2g:koi+, firefox26 wontfix, firefox27 wontfix, firefox28 fixed, b2g-v1.2 fixed)
People
(Reporter: atsai, Assigned: gerard-majax)
References
Details
Attachments
(2 files, 7 obsolete files)
30.32 KB,
image/png
|
Details | |
8.81 KB,
patch
|
gerard-majax
:
review+
|
Details | Diff | Splinter Review |
It's hard to dial to second person during a phone call. STR: 1. Make a phone call to anyone 2. Try to make second phone call to another one without hang up Expected Result: *. I can edit the number easily Actual result: *. It's hard to click on the delete button to edit the phone number. [Environment] Gaia: a22ba4a3a9efd99f94adf9ece8a2b391d4df295b Gecko: http://hg.mozilla.org/mozilla-central/rev/1fda74e33e06 BuildID 20130924040201 Version 27.0a1
Reporter | ||
Updated•11 years ago
|
blocking-b2g: --- → koi?
Comment 1•11 years ago
|
||
Likely a dupe, but this is a blocker.
Comment 2•11 years ago
|
||
Might be a dupe of bug 914917, but I need a dev to confirm that.
Assignee | ||
Comment 3•11 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #2) > Might be a dupe of bug 914917, but I need a dev to confirm that. From what I can reproduce, I'm not sure: bug 914917 seems to be correctly fixed in my gaia, yet I can reprodce the issue described here: I have to make sure to tap well below the center of the back button to make sure it does not enables the current call.
Comment 4•11 years ago
|
||
My assumption is that http://mxr.mozilla.org/mozilla-central/source/layout/base/PositionedEventTargeting.cpp#119 does not handle touch events but only mouse events. Adding a new method HasTouchListener that checks for touchstart/touchmove/touchend may help here. If this is the case please add bug 789358 as a blocking bug for this one.
Assignee | ||
Comment 5•11 years ago
|
||
Worst, after debugging it a bit with Vivien, we noticed that this might be much worse: when having a first call in place, switching to SMS app, and when tapping on the title bar of the SMS app, for example to write a new SMS, then it triggers the dialer. Also, we disabled event fluffing and this somehow fixed.
Assignee | ||
Comment 6•11 years ago
|
||
Adding a role=application attribute seems to fix the issue.
Comment 7•11 years ago
|
||
I assume that the fix we're targetting here will fix bug 909821 as well.
Blocks: 909821
Assignee | ||
Comment 8•11 years ago
|
||
Pointer to Github pull-request
Assignee | ||
Updated•11 years ago
|
Attachment #816565 -
Flags: review?(21)
Comment 9•11 years ago
|
||
Comment on attachment 816565 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/12816 Sounds good to me. Let's CC'ed one accessibility guy to check if this has a real meaning :)
Attachment #816565 -
Flags: review?(eitan)
Attachment #816565 -
Flags: review?(21)
Attachment #816565 -
Flags: review+
Updated•11 years ago
|
Component: Gaia::Dialer → Gaia::System
Updated•11 years ago
|
Whiteboard: DUPEME
Comment 10•11 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #4) > My assumption is that > http://mxr.mozilla.org/mozilla-central/source/layout/base/ > PositionedEventTargeting.cpp#119 does not handle touch events but only mouse > events. Adding a new method HasTouchListener that checks for > touchstart/touchmove/touchend may help here. Do you still think this? Then we should get a bug to make this change.
Comment 11•11 years ago
|
||
Comment on attachment 816565 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/12816 I'm not certain how this affects the fluffing heuristics. The problem from an accessibility standpoint, is that now we have two parent containers that are applications (the iframe, and the body of the actual app). We rely on the default iframe role to know that we may have reached a process boundary, and to start relaying messages. So changing the role is generally problematic here. Is there a way to do this without a role? If it absolutely has to be one, perhaps a 'region' role would be more fitting. But again, I would really prefer this were done in another way since it will interfere with the screen reader no matter what.
Attachment #816565 -
Flags: review?(eitan) → review-
Assignee | ||
Updated•11 years ago
|
Summary: [Dailer][UX] The dialer layout is hard to use while make a second phone call → [System][UX] The dialer layout is hard to use while make a second phone call
Assignee | ||
Comment 12•11 years ago
|
||
(In reply to Eitan Isaacson [:eeejay] from comment #11) > Comment on attachment 816565 [details] > Pointer to Github pull request: > https://github.com/mozilla-b2g/gaia/pull/12816 > > I'm not certain how this affects the fluffing heuristics. The problem from > an accessibility standpoint, is that now we have two parent containers that > are applications (the iframe, and the body of the actual app). We rely on > the default iframe role to know that we may have reached a process boundary, > and to start relaying messages. So changing the role is generally > problematic here. > > Is there a way to do this without a role? If it absolutely has to be one, > perhaps a 'region' role would be more fitting. But again, I would really > prefer this were done in another way since it will interfere with the screen > reader no matter what. Thanks for your feedback. We discussed of some other way to detect this case, so we might be able to address this issue without the whole role thing. I'm going to continue work on this.
Comment 13•11 years ago
|
||
I started work on making the dialer accessible in bug 923139, but will hold off on that work until this one is resolved. Perhaps you can even hijack that if you wish, I don't mind. :)
Blocks: 923139
Assignee | ||
Comment 14•11 years ago
|
||
(In reply to Marco Zehe (:MarcoZ) from comment #13) > I started work on making the dialer accessible in bug 923139, but will hold > off on that work until this one is resolved. Perhaps you can even hijack > that if you wish, I don't mind. :) I'll try and implement another solution, should be a matter of hours :)
Component: Gaia::System → Gaia::Dialer
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → lissyx+mozillians
Assignee | ||
Comment 15•11 years ago
|
||
Please find attached a new version of the patch, that makes uses of current elemtns rather than setting a new role.
Attachment #816565 -
Attachment is obsolete: true
Attachment #817145 -
Flags: review?(21)
Assignee | ||
Comment 16•11 years ago
|
||
Comment on attachment 817145 [details] [diff] [review] 0001-Bug-921928-Make-iframe-mozbrower-remote-a-preferred-.patch Removing review flag fow now, until I add some tests.
Attachment #817145 -
Flags: review?(21)
Assignee | ||
Comment 17•11 years ago
|
||
Please find a new version of this patch. Writing those tests took some time, but the tricky case is now clearly exposed !
Attachment #817145 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #817845 -
Flags: review?(etienne)
Attachment #817845 -
Flags: feedback?(21)
Comment 18•11 years ago
|
||
Comment on attachment 817845 [details] [diff] [review] 0001-Bug-921928-Make-iframe-mozbrower-remote-a-preferred-.patch Review of attachment 817845 [details] [diff] [review]: ----------------------------------------------------------------- Definitely not reviewable by me :) 21 is my best guess.
Attachment #817845 -
Flags: review?(etienne)
Attachment #817845 -
Flags: review?(21)
Attachment #817845 -
Flags: feedback?(21)
Comment 19•11 years ago
|
||
Comment on attachment 817845 [details] [diff] [review] 0001-Bug-921928-Make-iframe-mozbrower-remote-a-preferred-.patch Review of attachment 817845 [details] [diff] [review]: ----------------------------------------------------------------- f+ but the real reviewer should be roc I believe. ::: layout/base/tests/Makefile.in @@ +115,5 @@ > test_bug677878.html \ > test_bug696020.html \ > test_event_target_radius.html \ > + test_event_target_iframe.html \ > + test_event_target_iframe_apps.html \ What about adding _oop_ in the name of the apps file? ::: layout/base/tests/test_event_target_iframe.html @@ +81,5 @@ > + ["ui.mouse.radius.visitedweight", 50], > + ["dom.mozBrowserFramesEnabled", true], > + ] > + }, cont); > + SpecialPowers.addPermission("browser", true, document); You need to clean up all the prefs at the end of the test. So first let's get the values for those before altering them and reset them in endTest. @@ +132,5 @@ > + dialer.addEventListener('mousedown', function(e) {}); > + > + // This event listener is just here to record what iframe has been hit, > + // and sets the 'eventTarget' to the iframe's id value so that the > + // testMouseClick() code can correctly check. Can you add a comment explaining that you can not add it to the 'apps' div since it will altered the test. @@ +151,5 @@ > + testMouseClick("apps", 20, 1, "apps", "apps <iframe mozbrowser remote> hit for touch input", { > + inputSource: SpecialPowers.Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH > + }); > + > + showDebug(); Maybe you don't want to show debug here after it has landed? as you wish.
Attachment #817845 -
Flags: review?(roc)
Attachment #817845 -
Flags: review?(21)
Attachment #817845 -
Flags: feedback+
Assignee | ||
Comment 20•11 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #19) > Comment on attachment 817845 [details] [diff] [review] > 0001-Bug-921928-Make-iframe-mozbrower-remote-a-preferred-.patch > > Review of attachment 817845 [details] [diff] [review]: > ----------------------------------------------------------------- > > f+ but the real reviewer should be roc I believe. > > ::: layout/base/tests/Makefile.in > @@ +115,5 @@ > > test_bug677878.html \ > > test_bug696020.html \ > > test_event_target_radius.html \ > > + test_event_target_iframe.html \ > > + test_event_target_iframe_apps.html \ > > What about adding _oop_ in the name of the apps file? Nice idea, done. > > ::: layout/base/tests/test_event_target_iframe.html > @@ +81,5 @@ > > + ["ui.mouse.radius.visitedweight", 50], > > + ["dom.mozBrowserFramesEnabled", true], > > + ] > > + }, cont); > > + SpecialPowers.addPermission("browser", true, document); > > You need to clean up all the prefs at the end of the test. So first let's > get the values for those before altering them and reset them in endTest. With a « clear » request ? As far as I've checked, this is not done in others tests though :/ > > @@ +132,5 @@ > > + dialer.addEventListener('mousedown', function(e) {}); > > + > > + // This event listener is just here to record what iframe has been hit, > > + // and sets the 'eventTarget' to the iframe's id value so that the > > + // testMouseClick() code can correctly check. > > Can you add a comment explaining that you can not add it to the 'apps' div > since it will altered the test. Done. > > @@ +151,5 @@ > > + testMouseClick("apps", 20, 1, "apps", "apps <iframe mozbrowser remote> hit for touch input", { > > + inputSource: SpecialPowers.Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH > > + }); > > + > > + showDebug(); > > Maybe you don't want to show debug here after it has landed? as you wish. Done.
Assignee | ||
Comment 21•11 years ago
|
||
Please find an updated version that should address all of the previous remarks.
Attachment #817845 -
Attachment is obsolete: true
Attachment #817845 -
Flags: review?(roc)
Attachment #818456 -
Flags: review?(roc)
Comment on attachment 818456 [details] [diff] [review] 0001-Bug-921928-Make-iframe-mozbrower-remote-a-preferred-_hg.patch Review of attachment 818456 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/PositionedEventTargeting.cpp @@ +153,5 @@ > + content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::mozbrowser, > + nsGkAtoms::_true, eIgnoreCase) && > + content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::Remote, > + nsGkAtoms::_true, eIgnoreCase)) { > + return true; It doesn't really make sense to me that whether an <iframe> is remote affects how events are targeted. Can you explain what you're trying to do and why?
Assignee | ||
Comment 23•11 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22) > Comment on attachment 818456 [details] [diff] [review] > 0001-Bug-921928-Make-iframe-mozbrower-remote-a-preferred-_hg.patch > > Review of attachment 818456 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/PositionedEventTargeting.cpp > @@ +153,5 @@ > > + content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::mozbrowser, > > + nsGkAtoms::_true, eIgnoreCase) && > > + content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::Remote, > > + nsGkAtoms::_true, eIgnoreCase)) { > > + return true; > > It doesn't really make sense to me that whether an <iframe> is remote > affects how events are targeted. Can you explain what you're trying to do > and why? Basically, but I'm Ccing Vivien in case I'm wrong/incomplete, in the present case, we have two iframes: - one for the attention screen that is the notification of current call of the dialer, the little green bar at the top - one for others applications that the user runs In this case, the attention screen has a 'mousedown' event listener, which make it a good target. Without this change, when we tap on the application, which can be SMS, if you want to send a new SMS, then the event is targetted to the wrong <iframe>, i.e., the attention screen one. So the change re-equilibrates the heuristics. The added testcase exposes this issue by recreating two iframes, namely 'dialer' and 'apps' that are very close. We simulate touch/click events targetting inside the 'apps' iframe, but very close to the top of the iframe: it is one pixel below the top. What happens in this test case is the following: - if we disable remote/oop, we enable event fluffing and we remove the proposed change, then the test succeed and we tap inside the correct iframe - if we enable remote/oop, we disable event fluffing, and we remove the proposed change, then the test also succeed and we tap inside the correct iframe. - if we enable remote/oop, event fluffing but we still do not apply the proposed change, then we reproduce this bug, and when tapping one pixel below the top of the 'apps' iframe, then iframe that receives the 'mousedown' event is the 'dialer' and not 'apps'. - with remote/oop, event fluffing, and the proposed change, then the test goes back green. Also, while testing this change with vivien on an Inari device, we noticed that not only the reported bug was fixed, but a bunch of mistapped events, especially one that involves tapping the 'Send' button to send a SMS when there is a keyboard suggestion just below did not trigger selecting the suggestion anymore, but sending the SMS. I hope this clarifies the change :)
Flags: needinfo?(21)
Comment 24•11 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22) > Comment on attachment 818456 [details] [diff] [review] > 0001-Bug-921928-Make-iframe-mozbrower-remote-a-preferred-_hg.patch > > Review of attachment 818456 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/PositionedEventTargeting.cpp > @@ +153,5 @@ > > + content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::mozbrowser, > > + nsGkAtoms::_true, eIgnoreCase) && > > + content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::Remote, > > + nsGkAtoms::_true, eIgnoreCase)) { > > + return true; > > It doesn't really make sense to me that whether an <iframe> is remote > affects how events are targeted. Can you explain what you're trying to do > and why? In short I think this is because the event dispatching code in the parent process does not check the content of the remote iframe when looking for a target to the event, but instead it compares: (a) <iframe mozbrowser remote id="dialer" onmousedown> (b) <iframe mozbrowser remote id="apps"> It ends up deciding that (a) is more interesting because it has a mouse event handler and dispatch the event to frame (a). The reason because it does not compare the content of the iframes is because it does not have access to the content of that frame without dispatching the event to the child process. Which is something that is not supported by the fluffling code afaik.
Flags: needinfo?(21)
Comment 25•11 years ago
|
||
This bug also makes it harder to hit the back button since the statusbar will be preferred while resolving the click.
Comment on attachment 818456 [details] [diff] [review] 0001-Bug-921928-Make-iframe-mozbrower-remote-a-preferred-_hg.patch Review of attachment 818456 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/PositionedEventTargeting.cpp @@ +153,5 @@ > + content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::mozbrowser, > + nsGkAtoms::_true, eIgnoreCase) && > + content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::Remote, > + nsGkAtoms::_true, eIgnoreCase)) { > + return true; OK, please add a comment that we don't have access to the content of remote iframes therefore the usual fluffing code doesn't work for remote iframe content, and we have to be optimistic and assume that the iframe needs to be a target.
Attachment #818456 -
Flags: review?(roc) → review+
Assignee | ||
Comment 27•11 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26) > Comment on attachment 818456 [details] [diff] [review] > 0001-Bug-921928-Make-iframe-mozbrower-remote-a-preferred-_hg.patch > > Review of attachment 818456 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/PositionedEventTargeting.cpp > @@ +153,5 @@ > > + content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::mozbrowser, > > + nsGkAtoms::_true, eIgnoreCase) && > > + content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::Remote, > > + nsGkAtoms::_true, eIgnoreCase)) { > > + return true; > > OK, please add a comment that we don't have access to the content of remote > iframes therefore the usual fluffing code doesn't work for remote iframe > content, and we have to be optimistic and assume that the iframe needs to be > a target. Thanks, I'm adding this: // Bug 921928: we don't have access to the content of remote iframe. // So fluffing won't go there. We do an optimistic assumption here: // that the content of the remote iframe needs to be a target. Are you okay with this, or do you want more explanations ?
Flags: needinfo?(roc)
That's OK. Thanks!!!
Flags: needinfo?(roc)
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 29•11 years ago
|
||
This patch is the updated version including the comments to describe more closely the issue. HG and checkin ready.
Attachment #818456 -
Attachment is obsolete: true
Attachment #820309 -
Flags: review+
Comment 30•11 years ago
|
||
qimport fails to parse this patch properly - it's partly in the email format, partly not. Please could you follow https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F and reupload? :-)
Keywords: checkin-needed
Assignee | ||
Comment 31•11 years ago
|
||
Please find a new copy without the '[PATCH]' header
Attachment #820309 -
Attachment is obsolete: true
Attachment #821062 -
Flags: review+
Comment 32•11 years ago
|
||
Ah that works, I'm guessing the parser for qimport switches to email-format mode if it sees the prefix. Thank you :-) Trees are closed for now, so marking checkin-needed for later (I'm UTC+0), so this will likely be tomorrow.
Updated•11 years ago
|
Keywords: checkin-needed
Updated•11 years ago
|
Target Milestone: --- → 1.2 C4(Nov8)
Comment 33•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6731046b796 :-)
Keywords: checkin-needed
Comment 34•11 years ago
|
||
Backed out for mochitest-4 timeouts in test_event_target_iframe_apps_oop.html, eg: https://tbpl.mozilla.org/php/getParsedLog.php?id=29661876&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=29662193&tree=Mozilla-Inbound remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8803d874cbea
Assignee | ||
Comment 35•11 years ago
|
||
(In reply to Ed Morley [:edmorley UTC+1] from comment #34) > Backed out for mochitest-4 timeouts in > test_event_target_iframe_apps_oop.html, eg: > https://tbpl.mozilla.org/php/getParsedLog.php?id=29661876&tree=Mozilla- > Inbound > https://tbpl.mozilla.org/php/getParsedLog.php?id=29662193&tree=Mozilla- > Inbound > > remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8803d874cbea Gasp, that's my fault, silly file which is not a test ... I'll fix it asap.
Assignee | ||
Comment 36•11 years ago
|
||
Patch is updated, I'll run tests and it should be here soon :)
Comment 37•11 years ago
|
||
Alexandre, you can attach your new patch here and I can trigger a try build if you want. And you should follow http://www.mozilla.org/hacking/committer/ to get a level 1 access so that you can do try by yourself ;)
Assignee | ||
Comment 38•11 years ago
|
||
Please find an updated version of the patch that should pass all tests now. I've tested it on the full set of layout/base tests on my laptop, no failure reported.
Attachment #821062 -
Attachment is obsolete: true
Attachment #822928 -
Flags: review+
Assignee | ||
Comment 39•11 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #37) > Alexandre, you can attach your new patch here and I can trigger a try build > if you want. > And you should follow http://www.mozilla.org/hacking/committer/ to get a > level 1 access so that you can do try by yourself ;) I'd be honored if you could launch a try build for me :).
Comment 40•11 years ago
|
||
try is https://tbpl.mozilla.org/?tree=Try&rev=6f546082374e I asked for all builds, mochitests, and some other tests.
Assignee | ||
Comment 41•11 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #40) > try is https://tbpl.mozilla.org/?tree=Try&rev=6f546082374e > > I asked for all builds, mochitests, and some other tests. Looks like everything is getting green :)
Keywords: checkin-needed
Comment 42•11 years ago
|
||
Or not. https://tbpl.mozilla.org/php/getParsedLog.php?id=29741739&tree=Try
Keywords: checkin-needed
Assignee | ||
Comment 43•11 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4][PTO 10/19 - 10/28] from comment #42) > Or not. > https://tbpl.mozilla.org/php/getParsedLog.php?id=29741739&tree=Try Damn, you're right, I got too fast. I'll look closer at this tomorrow :)
Assignee | ||
Comment 44•11 years ago
|
||
Please find an updated version that should pass all tests now. Julien, I'd be very pleased if you could do a try build for me :)
Attachment #822928 -
Attachment is obsolete: true
Attachment #823336 -
Flags: review+
Flags: needinfo?(felash)
Comment 45•11 years ago
|
||
new try: https://tbpl.mozilla.org/?tree=Try&rev=4fadd340d048
Flags: needinfo?(felash)
Assignee | ||
Comment 46•11 years ago
|
||
Everything is green now, see https://tbpl.mozilla.org/?tree=Try&rev=4fadd340d048
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 47•11 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/5a6f1071c7ae
Keywords: checkin-needed
Comment 48•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5a6f1071c7ae
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 49•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/3ba96421ba4a
status-b2g-v1.2:
--- → fixed
status-firefox26:
--- → wontfix
status-firefox27:
--- → wontfix
status-firefox28:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•