Closed Bug 1144707 Opened 5 years ago Closed 4 years ago

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

Categories

(Firefox for Android :: Awesomescreen, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox36 --- affected
firefox37 --- affected
firefox38 --- affected
firefox39 --- affected
firefox40 --- fixed
firefox44 --- verified
fennec + ---

People

(Reporter: cos_flaviu, Assigned: sergej, Mentored)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [lang=java])

Attachments

(3 files, 3 obsolete files)

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.
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.
tracking-fennec: --- → ?
Mentor: michael.l.comella
tracking-fennec: ? → +
/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 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
Attachment #8584782 - Flags: review?(mhaigh)
Assignee: nobody → michael.l.comella
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.
Attachment #8584782 - Flags: review?(mhaigh) → review+
https://hg.mozilla.org/mozilla-central/rev/44fbf043ef85
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
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).
Flags: needinfo?(michael.l.comella)
That's... bizarre. Low pri, so I'm backing this out.
Flags: needinfo?(michael.l.comella)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/fa5c609b2942
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
The push in comment 10 was a backout - sorry if that was unclear.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Not actively working on this.
Assignee: michael.l.comella → nobody
Whiteboard: [lang=java]
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.
Attachment #8665447 - Flags: review?(michael.l.comella)
(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.
Assignee: nobody → sergej
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
Attachment #8665447 - Flags: review?(michael.l.comella) → review+
Attachment #8665447 - Attachment is obsolete: true
Keywords: checkin-needed
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
Flags: needinfo?(sergej)
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
Sorry, probably I missed that build error or looked on another bug report. Can you rerun build test? Thanks
Attachment #8665725 - Attachment is obsolete: true
Flags: needinfo?(sergej)
Attachment #8666145 - Flags: review?(michael.l.comella)
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/
Keywords: checkin-needed
Attachment #8666145 - Flags: review?(michael.l.comella) → review+
(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
Flags: needinfo?(michael.l.comella)
https://hg.mozilla.org/mozilla-central/rev/441f0297151a
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 40 → Firefox 44
(In reply to Sergej Kravcenko from comment #25)
> Thank you. Applied in Bug 1209439

All done! Thanks again Sergej!
Flags: needinfo?(michael.l.comella)
Verified as fixed on latest Nightly build (44.0a1 / 2015-10-08) on Nexus 9 with Android 5.1.1
fwiw, this is caused by bug 1216114 – I filed bug 1216212 to remove this hack once bug 1216114 lands.
You need to log in before you can comment on or make changes to this bug.