1144707 - Tapping in between the three dot menu button and the edge of the screen will enable edit mode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Flaviu Cos]

Attachment: three dot menu button.jpg

Environment: 
Device: Nexus 7 (Android 5.0.2);
Build: Nightly 39.0a1 (2015-03-18);

Steps to reproduce:
1. Launch Fennec;
2. Tap in between the three dot menu button and the edge of the screen.

Expected result:
The options menu is triggered.

Actual result:
Edit mode is enabled and the URL bar gets in focus (the same behavior as tapping on the URL bar).

Notes:
Please check the attached screenshot.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I believe there is a margin to the right of the three-dot menu button - that could become padding.

However, I wonder why the hit area for the toolbar is that big - I'd expect it just to be the display/edit area.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Precedent: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabs/TabStrip.java?rev=5e6ac1912e5a#69

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: bz://1144707/mcomella

/r/6217 - Bug 1144707 - Add touch delegate to cover the edge of the screen and the menu button in toolbar. r=mhaigh

Pull down this commit:

hg pull review -r 61ece49c1d1301e10195cd4104f3c56d0b2abf23

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8584782 [details]
MozReview Request: bz://1144707/mcomella

/r/6217 - Bug 1144707 - Add touch delegate to cover the edge of the screen and the menu button in toolbar. r=mhaigh

Pull down this commit:

hg pull review -r 61ece49c1d1301e10195cd4104f3c56d0b2abf23

Comment 5

[Martyn Haigh (:mhaigh)]

Comment on attachment 8584782 [details]
MozReview Request: bz://1144707/mcomella

https://reviewboard.mozilla.org/r/6215/#review5385

Ship It!

::: mobile/android/base/toolbar/BrowserToolbarTablet.java
(Diff revision 1)
> +        getViewTreeObserver().addOnPreDrawListener(new ViewTreeObserver.OnPreDrawListener() {

Add a comment as to why this block is in a onPreDrawListener.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/44fbf043ef85

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/44fbf043ef85

Comment 8

[Flaviu Cos]

The fix created another issue. After tapping between the three dot menu button and the edge of the screen the options menu will pop up, but after that tapping on the URL bar will work as long tap (context menu is triggered).
Tested on latest nightly 2015-04-03 using Nexus 7 (Android 5.1).

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

That's... bizarre. Low pri, so I'm backing this out.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/fa5c609b2942

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fa5c609b2942

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The push in comment 10 was a backout - sorry if that was unclear.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1144707 - Add touch delegate to cover the edge of the screen and the menu button in toolbar. r=mhaigh

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Not actively working on this.

Comment 16

[Sergej Kravcenko]

Attachment: replace button margin with view to dismiss clicks

One of the possible solutions. I think it's better than confusing touch routing inside the code.

The other solution is to add additional style with padding for menu button, but this can be harder to maintain later.  

I don't know the purpose of that 6dp margin and why it is used only with menu button, but not with tab button. If we always add that 6dp margin to the right, we can also remove code in BrowserToolbarTabletBase.java and leave only margin view.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Sergej Kravcenko from comment #16)
> I don't know the purpose of that 6dp margin and why it is used only with
> menu button, but not with tab button.

The margin is used to make the pressed state not press against the edge of the screen. The margins are unnecessary between items because two items are never pressed at the same time so it doesn't matter if there's no margin between the pressed states of the items.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8665447 [details] [diff] [review]
replace button margin with view to dismiss clicks

Review of attachment 8665447 [details] [diff] [review]:
-----------------------------------------------------------------

Nice and simple – I like it!

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74d39a828032

If you make the changes suggested below and upload a new patch, you can add "checkin-needed" without re-review.

::: mobile/android/base/resources/layout-large-v11/browser_toolbar.xml
@@ +109,5 @@
>                          android:layout_marginLeft="16dp"
>                          android:layout_marginRight="16dp"
>                          android:background="@drawable/tabs_count"/>
>  
> +    <View android:id="@+id/menu_margin"

Add a comment explaining this is here to extend the hit target and that my previous approach using whatever API didn't work.

@@ +137,5 @@
>  
>      </org.mozilla.gecko.widget.themed.ThemedFrameLayout>
>  
> +
> +

nit: we try not to add excessive whitespace

Comment 19

[Sergej Kravcenko]

Attachment: replace button margin with view to dismiss clicks

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a362959c7f52

Comment 21

[Carsten Book [:Tomcat]]

Hi, had to back this out in https://hg.mozilla.org/integration/fx-team/rev/39a4d02f17db seems one of this changes caused a bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=4833952&repo=fx-team

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Oh! We ship two APKs – a regular version and a slimmed down version for Android 2.3. Notably, the layout-large* files are excluded from 2.3 builds so the id is missing when trying to build on 2.3. You should add a dummy view in the corresponding layout/browser_toolbar.xml file to compensate. Does that make sense?

There are a few classes that are also excluded from 2.3 builds [1] so you don't need to worry about that restriction there.

I see the try push I made went red on 2.3. Before adding checkin-needed, you should verify that the push is green before adding `checkin-needed`.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/moz.build?rev=df3e2b33e4bc#626

Comment 23

[Sergej Kravcenko]

Attachment: menu button margin view

Sorry, probably I missed that build error or looked on another bug report. Can you rerun build test? Thanks

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff8603ac1842

By the way, if you wanted to make your own pushes to our try test servers, I'd be happy to vouch for level 1 access for you. See [1] if you're interested.

[1]: https://www.mozilla.org/en-US/about/governance/policies/commit/

Comment 25

[Sergej Kravcenko]

(In reply to Michael Comella (:mcomella) from comment #24)
> try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff8603ac1842
> 
> By the way, if you wanted to make your own pushes to our try test servers,
> I'd be happy to vouch for level 1 access for you. See [1] if you're
> interested.
> 
> [1]: https://www.mozilla.org/en-US/about/governance/policies/commit/


Thank you. Applied in Bug 1209439

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/441f0297151a

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/441f0297151a

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Sergej Kravcenko from comment #25)
> Thank you. Applied in Bug 1209439

All done! Thanks again Sergej!

Comment 29

[u549602]

Verified as fixed on latest Nightly build (44.0a1 / 2015-10-08) on Nexus 9 with Android 5.1.1

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

fwiw, this is caused by bug 1216114 – I filed bug 1216212 to remove this hack once bug 1216114 lands.