1154642 - [System] context menu doesn't focus to correct element and doesn't get focus while relaunch the app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

[John Hu [:johnhu][:johu][:醬糊小弟]]

Symptom: wrong focused element

STR of part 1:
1. open uitest app
2. open context menu  <== focus is at body since we had call app.blur()

STR of part 2:
1. open uitest app
2. open context menu
3. tap background of context menu to get focus back to context menu
4. press home
5. open uitest app again  <== focus is at home app

Expected:
the focus should be cancel button.

Comment 1

[John Hu [:johnhu][:johu][:醬糊小弟]]

The patch should apply to:

1. ContextMenuView to focus the correct element
2. BrowserMixin/AppWindow to focus context menu while context menu is shown

So, this bug depends on bug 1154195.

Comment 2

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: interest finding

While fixing this bug, I try to focus the cancel button of context menu. According to the information we found at [1], we use the attached patch(simplified version) to focus it. But the focus change failed in this case.

STR:
1. open uitest app
2. open context menu  <== focus is still at body after we call this.elements.cancel.focus()


After some investigation, we found that the element of context menu uses "transition" on "visibility" attribute and it prevents us to focus[2]. We shouldn't transit "visibility" attribute and it doesn't make sense. Once we remove it, it works correctly. That line is added by bug 943657 which doesn't explain why. But it looks like a small misunderstanding of transition.

I will remove that in the patch.


[1] https://wiki.mozilla.org/FirefoxOS/Stingray/Focus-Manager
[2] https://github.com/mozilla-b2g/gaia/blob/eff65532a7b5a4edca995cf05b8f3ec5b56aaafa/apps/system/style/action_menu/action_menu_extended.css#L52

Comment 3

[John Hu [:johnhu][:johu][:醬糊小弟]]

WIP patch is at: https://github.com/huchengtw-moz/gaia/commit/c9b1ea9c4f141cdccd563431978abfe2ded9eb04

Since this bug depends on others, I will wait for them.

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] huchengtw-moz:bug-1154642-context-menu-focus > mozilla-b2g:master

Comment 5

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8593785 [details] [review]
[gaia] huchengtw-moz:bug-1154642-context-menu-focus > mozilla-b2g:master

This patch change:
1. let focus in browser_mixin.js to focus context menu
2. HierarchyManager focus top most UI while no module given.
3. remove the non-sense visibility transition.

Please review this patch. Thanks.

Comment 6

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8593785 [details] [review]
[gaia] huchengtw-moz:bug-1154642-context-menu-focus > mozilla-b2g:master

Removing r? for deciding whether or not to add integration tests and how to run it without touch.

Comment 7

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8593785 [details] [review]
[gaia] huchengtw-moz:bug-1154642-context-menu-focus > mozilla-b2g:master

Integration test is included.

Please see comment 6 and review this patch. Thanks.

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8593785 [details] [review]
[gaia] huchengtw-moz:bug-1154642-context-menu-focus > mozilla-b2g:master

r=me, but we need to be careful about the possible performance issue (if request(focus) is called multiple times)

Comment 9

[John Hu [:johnhu][:johu][:醬糊小弟]]

merge to master:
https://github.com/mozilla-b2g/gaia/commit/7856cbd44e0c0c2164f3d0896f695645788b0b2f

tree herder is all green:
https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=7a5b680ffc05f57920b38806491356ee7cb7ec9b

Comment 10

[John Hu [:johnhu][:johu][:醬糊小弟]]

backed out because of bug 1158254.

941ca22724eca93111b94433cc2bc19b2a99002f

Comment 11

[John Hu [:johnhu][:johu][:醬糊小弟]]

The root cause of bug 1158254 regression is that we try to focus the cancel button to early while transiting. Once the issue happens, the scroll top of <div id="windows"...> is 555 instead of 0. The issue is gone when we change the scroll top to 0 or delayed the focus to 300ms.

I will change the patch to wait for transitionend before setting the focus.

Comment 12

[GitHub Autolander Bot]

Attachment: [gaia] huchengtw-moz:bug-1154642-v2 > mozilla-b2g:master

Comment 13

[John Hu [:johnhu][:johu][:醬糊小弟]]

The patch is ready. I will wait for tree herder before setting review flag.

Comment 14

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8593785 [details] [review]
[gaia] huchengtw-moz:bug-1154642-context-menu-focus > mozilla-b2g:master

>https://github.com/mozilla-b2g/gaia/pull/29563

Comment 15

[John Hu [:johnhu][:johu][:醬糊小弟]]

BTW, I had added another integration test case for testing bug 1158254.

Comment 16

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8597896 [details] [review]
[gaia] huchengtw-moz:bug-1154642-v2 > mozilla-b2g:master

Alive,

Sorry for producing regression. I had updated the patch to wait for the transition end before requesting focus. The scroll top change is caused by focus() when the button is not inside of screen.

Comment 17

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #16)
> Comment on attachment 8597896 [details] [review]
> [gaia] huchengtw-moz:bug-1154642-v2 > mozilla-b2g:master
> 
> Alive,
> 
> Sorry for producing regression. I had updated the patch to wait for the
> transition end before requesting focus. The scroll top change is caused by
> focus() when the button is not inside of screen.

I am not sure I like the setTimeout here.. let's talk about this tomorrow.

Comment 18

[John Hu [:johnhu][:johu][:醬糊小弟]]

setTimeout is for getting rid of Keyboard Event processing. Gecko will prevent the focus change while the the function call is called at Keyboard Event processing cycle. Please find the comment 4, 9, and 13 in bug 1113592 for the main reason.

Comment 19

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #18)
> setTimeout is for getting rid of Keyboard Event processing. Gecko will
> prevent the focus change while the the function call is called at Keyboard
> Event processing cycle. Please find the comment 4, 9, and 13 in bug 1113592
> for the main reason.

You should comment this.

Also, is there no regular way to fix this in gecko? Do we need to setTimeout everywhere when we want to focus something in system app?

Comment 20

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #19)
> You should comment this.

Sure, I will do that.

> 
> Also, is there no regular way to fix this in gecko? Do we need to setTimeout
> everywhere when we want to focus something in system app?

This is not an issue but a feature created by bug 552255. I will add the bug number to setTimeout. In smart-system app, we do that in focus manager which is the hierarchy manager in system app. I don't have want to add setTimeout to hierarchy manager because I don't want to change too much in a patch.

Comment 21

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #20)
> This is not an issue but a feature created by bug 552255. I will add the bug
> number to setTimeout. In smart-system app, we do that in focus manager which
> is the hierarchy manager in system app. I don't have want to add setTimeout
                                                  ^^^^^^^^^ Typo here...

I don't want to add setTimeout to hierarchy manager because I don't want to change too much in this patch.

Comment 22

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8597896 [details] [review]
[gaia] huchengtw-moz:bug-1154642-v2 > mozilla-b2g:master

I believe the right fix is inside HierarchyManager but let's see what will happen with this local fix.

BTW, I think the original visibility transition means it wants to transit from opacity 0 to 1. It did not work because we need to RAF before doing that.

Comment 23

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #22)
> BTW, I think the original visibility transition means it wants to transit
> from opacity 0 to 1. It did not work because we need to RAF before doing
> that.

Should we file another bug for this one? It's not hard to fix it without RAF.

Comment 24

[John Hu [:johnhu][:johu][:醬糊小弟]]

merged to master:
https://github.com/mozilla-b2g/gaia/commit/e3fc645f43d71a373cf80b10f771a2a3808a0096

tree herder is all green:
https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=ddab00fde5ce40c6e2d3b07e9c726ae4151d51cb