Closed Bug 921928 Opened 6 years ago Closed 6 years ago

[System][UX] The dialer layout is hard to use while make a second phone call

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:koi+, firefox26 wontfix, firefox27 wontfix, firefox28 fixed, b2g-v1.2 fixed)

RESOLVED FIXED
1.2 C4(Nov8)
blocking-b2g koi+
Tracking Status
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
Attached image 2013-09-30-11-17-58.png
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
Keywords: polish
blocking-b2g: --- → koi?
Likely a dupe, but this is a blocker.
blocking-b2g: koi? → koi+
Keywords: polish
Whiteboard: DUPEME
Might be a dupe of bug 914917, but I need a dev to confirm that.
(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.
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.
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.
Blocks: 789358
Adding a role=application attribute seems to fix the issue.
I assume that the fix we're targetting here will fix bug 909821 as well.
Blocks: 909821
Pointer to Github pull-request
Attachment #816565 - Flags: review?(21)
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+
Component: Gaia::Dialer → Gaia::System
Whiteboard: DUPEME
(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 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-
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
(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.
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
(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: nobody → lissyx+mozillians
Depends on: 926389
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)
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)
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
Attachment #817845 - Flags: review?(etienne)
Attachment #817845 - Flags: feedback?(21)
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 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+
(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.
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?
(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)
(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)
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+
(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)
Keywords: checkin-needed
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+
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
Please find a new copy without the '[PATCH]' header
Attachment #820309 - Attachment is obsolete: true
Attachment #821062 - Flags: review+
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.
Keywords: checkin-needed
Target Milestone: --- → 1.2 C4(Nov8)
(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.
Patch is updated, I'll run tests and it should be here soon :)
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 ;)
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+
(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 :).
try is https://tbpl.mozilla.org/?tree=Try&rev=6f546082374e

I asked for all builds, mochitests, and some other tests.
(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
(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 :)
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)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5a6f1071c7ae
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 909821
You need to log in before you can comment on or make changes to this bug.