1217196 - Toolbar is not highlighted when pressed, only on toolbar expansion animation

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

This is especially noticeable when long-pressing where the toolbar is not highlighted at all.

I can only repro in my local builds but I can't repro on the Nightly from 10/21 – maybe a regression from bug 1216114, which seems to have landed between then?

Sergej, what do you think?

Comment 1

[Sergej Kravcenko]

I will double check this. I've already told Sebastian that this menu change can lead to regression bugs.

Comment 2

[Sergej Kravcenko]

When the toolbar must be highlighted? It is only highlighted when in edit mode or I'm missing something.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

I don't really get what a toolbar highlight should be? Is it that the EditText for the URL does not get focus while long-pressing? It did before but should it? I tried to find other examples but I can't find a more or less official app that has a long-press registered and such a strong focus highlight. For all other interactions (selection, editing text, ..) the state is correct, right?

Comment 4

[Sergej Kravcenko]

The only problem I can reproduce now is toolbar highlight when you touch the corners, because they are not covered by the controls and the main toolbar layout view is configured as clickable. It's easy to fix by making view not clickable, but I'm still checking focus behaviour. Hope I'll have time to finish it today.

Comment 5

[Sergej Kravcenko]

I've checked the keyboard control. Now it's totally broken for the toolbar, arrow key navigation jumps between invisible controls. When you move from the url edit box you can't return back. On phones you can't navigate from the url to buttons by pressing right arrow and etc.
Context menu changes added even more problems, now you can't activate url bar.

Do we need to fix these problems?

Comment 6

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

I filed this bug to recreate the previous behavior, which, iirc, is that pressing the EditText should highlight the EditText, like how pressing most Views in Android triggers a highlighted "pressed" state. This highlighted state remains as the toolbar expands and the EditText is editable. Similarly, long-presses should trigger the highlight.

I don't think we actively support keyboard navigation, however, I'm more concerned about access via accessibility tools (which generally use the same systems) – I filed bug 1218494 to write some tests to prevent this from happening in the future.

I think we have two options:
  * Back out bug 1216114 and directly fix the issues outlined in bug 1216114 comment 0
  * Don't back out & fix these regressions (though make sure they're regressions because I'm not sure if anyone actively tests these – maybe we should get some automated tests)

Sergej, do you know how much work it'd be to fix the regressions from bug 1216114? Do you have an opinion on which path we should take?

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Michael Comella (:mcomella) from comment #6)
> I filed this bug to recreate the previous behavior, which, iirc, is that
> pressing the EditText should highlight the EditText, like how pressing most
> Views in Android triggers a highlighted "pressed" state. This highlighted
> state remains as the toolbar expands and the EditText is editable.
> Similarly, long-presses should trigger the highlight.

This is just about the long-press, right? If the EditText is selected then we show a highlight. I'm not able to find any other Android app that shows a pressed state for an EditText.

> I don't think we actively support keyboard navigation, however, I'm more
> concerned about access via accessibility tools (which generally use the same
> systems) – I filed bug 1218494 to write some tests to prevent this from
> happening in the future.

Keyboard navigation/highlight still works. I can jump to the EditText with tab and it is highlighted. Even though that is not easy because you never know where you end up with the keyboard currently.. :)

> Sergej, do you know how much work it'd be to fix the regressions from bug
> 1216114? Do you have an opinion on which path we should take?

The only thing we lost is highlight on (long-)press. I'm not convinced that is should be there?

Comment 8

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(In reply to Sebastian Kaspari (:sebastian) from comment #7)
> This is just about the long-press, right?

No, press and long-press.

> If the EditText is selected then
> we show a highlight. I'm not able to find any other Android app that shows a
> pressed state for an EditText.

Sorry, I was using the wrong terminology – it's not actually the EditText until the toolbar expands.

The white space in our toolbar acts more like a button than an EditText because it expands before becoming the EditText – we should probably make it act like an other button before that time.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Michael Comella (:mcomella) from comment #8)
> The white space in our toolbar acts more like a button than an EditText
> because it expands before becoming the EditText – we should probably make it
> act like an other button before that time.

But isn't this more a technical detail? I'd assume that for users it's just an input that they can click and write into.

What I just would like to avoid is to go back to handling all kinds of touch events manually on the toolbar and then introduce more and more custom touch handling code just to fix things, like here:
https://reviewboard.mozilla.org/r/21581/diff/1#index_header

I'm not sure if it's worth it?

Comment 10

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(In reply to Sebastian Kaspari (:sebastian) from comment #9)
> But isn't this more a technical detail? I'd assume that for users it's just
> an input that they can click and write into.

I see that EditTexts on Android don't show have a visible interaction but textareas (in html, in our browser) do. Additionally, buttons on Android do – I suppose this is really up to the perception of the user.

Anthony, if you press or long-press the urlbar in the latest Nightly, there is only a highlight when it expands, whereas before (try aurora or older), it highlighted when pressed – do you feel strongly about which behavior is optimal? If not, it's less work to do the former.

Alternative thought: maybe we should try to move to a widget within platform convention (e.g. Material).

Comment 11

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I just tried this on my Nightly.

Do we know why this is happening? I'd prefer it to maintain the current UX if possible. I think the orange highlight was a nice touch to tell the user what they're long-pressing.

Or, would it be easier if we just expanded and activated the URL bar on long-press (at the same time) as we brought up the dialog for "Paste & Go"?

Comment 12

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(In reply to Anthony Lam (:antlam) from comment #11)
> Do we know why this is happening?

We changed the code under the hood to be more correct technically, but it changed the implementation slightly.

> Or, would it be easier if we just expanded and activated the URL bar on
> long-press (at the same time) as we brought up the dialog for "Paste & Go"?

If we kept the toolbar expanded after long-pressing, it'd probably be easier but I think it isn't the best experience for the "Copy address" or "Paste & Go" items, for example.

NI sebastian to get some movement here. Sergej, you're welcome to go at it too! :) Keep in mind this change is in 44.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

I wonder if we can keep the current/changed implementation and add the pressed state to the text field (instead of let the whole toolbar handle it). That would also match what you see: The input has a pressed state. But I have to dig more into this part of the code to understand how it is currently done. I fear that this is related to the expand animation and not so easy after all.

Sergej, let me know if you want to look into this too.

Let's back out bug 1216114 in 44 if this will require more work here.

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Okay, this is a bit tricky. The thing actually showing the pressed state is an ImageView [1]. It receives the pressed state via duplicateParentState from BrowserToolbar whenever it is pressed. This link is now broken. Touch events are now handled by ToolbarDisplayLayout. This view is on the same level like the ImageView so we cannot use duplicateParentState or addStatesFromChildren to delegate the pressed state. Manually delegating the touch events might be possible but is hacky too.

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/browser_toolbar.xml#10-21
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/ToolbarDisplayLayout.java#243-288

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1217196 - (Part 1) Move context menu and click handler back to BrowserToolbar. r?mcomella

Bug 1217196 - (Part 1) Move context menu and click handler back to BrowserToolbar. r?mcomella

This is more or less reverting the changes introduced in bug 1216114.

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1217196 - (Part 2) Remove touch event handling from BrowserToolbar. r?mcomella

Bug 1217196 - (Part 2) Remove touch event handling from BrowserToolbar. r?mcomella

This code seems to have handled scroll events that started on the toolbar in the past.
But this does not work anymore (for quite some time)  and in fact breaks touch event
handling (See bug 1046591 for a side-effect of only partially consuming events here).

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

I tried various things (Re-grouping the layouts to get duplicateParentState working again.., moving click handlers and pressed state to "UrlBarEntry" only without state delegation, ...) but this either creates layout issues or the state is not handled correctly because multiple views need to react to it.

It turns out that the issue bug 1216114 tried to solve (actually bug 1046591) was created by the custom touch event handling / suppression in BrowserToolbar. This code does not seem to have any effect anymore. This patch series restores the previous behavior and removes the touch handling code: The highlight is drawn again and the weird behavior from bug 1046591 is gone too.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8683687 [details]
MozReview Request: Bug 1217196 - (Part 1) Move context menu and click handler back to BrowserToolbar. r?mcomella

https://reviewboard.mozilla.org/r/24395/#review21985

If this reverts the previous behavior, I'd prefer you to back out the changeset in the previous bug so the change history is more clear. In the case of direct backouts, it also shouldn't need a review.

If this change included any small follow-up changes, you can flag me for a review again here.

Comment 19

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

If we backout, we can also requisition this bug to remove the unused code, rather than being about a bug fixed by the backout.

Comment 20

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8683688 [details]
MozReview Request: Bug 1217196 - (Part 2) Remove touch event handling from BrowserToolbar. r?mcomella

https://reviewboard.mozilla.org/r/24405/#review21987

Nice find.

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8683687 [details]
MozReview Request: Bug 1217196 - (Part 1) Move context menu and click handler back to BrowserToolbar. r?mcomella

A backout does not compile anymore (Unused code has been removed from BrowserToolbar. This unused code is partially restored by a backout). I can either modify the backout commit or land this patch.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8683687 [details]
MozReview Request: Bug 1217196 - (Part 1) Move context menu and click handler back to BrowserToolbar. r?mcomella

https://reviewboard.mozilla.org/r/24395/#review22409

Skimmed it to verify it was mostly a backout of bug 1216114 – lgtm.

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/ca651abc37c9394558e69e99c76bb6a54eedabbe
Bug 1217196 - (Part 1) Move context menu and click handler back to BrowserToolbar. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/73a7668db89dc3d04db6b1c47df12f8fcf5fb6a3
Bug 1217196 - (Part 2) Remove touch event handling from BrowserToolbar. r=mcomella

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

NI myself: This needs to be uplifted.

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/ca651abc37c9
https://hg.mozilla.org/mozilla-central/rev/73a7668db89d

Comment 27

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8683687 [details]
MozReview Request: Bug 1217196 - (Part 1) Move context menu and click handler back to BrowserToolbar. r?mcomella

Approval Request Comment

[Feature/regressing bug #]: Regression has been introduced by bug 1216114. This should be uplifted along with the follow-up bug 1224175.

[User impact if declined]: Pressed state (orange highlight) of toolbar is not always shown.

[Describe test coverage new/current, TreeHerder]: Local testing. Patch is on Nightly.

[Risks and why]: Very low - I can't really think of any risks here. Nightly should uncover them pretty fast.

[String/UUID change made/needed]: -

Comment 28

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8683688 [details]
MozReview Request: Bug 1217196 - (Part 2) Remove touch event handling from BrowserToolbar. r?mcomella

See previous comment.

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8683687 [details]
MozReview Request: Bug 1217196 - (Part 1) Move context menu and click handler back to BrowserToolbar. r?mcomella

The fix is mostly a backout so should be safe to uplift to Aurora44.

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8683688 [details]
MozReview Request: Bug 1217196 - (Part 2) Remove touch event handling from BrowserToolbar. r?mcomella

See my last comment. Aurora44+

Comment 31

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/d3e14f59b635
https://hg.mozilla.org/releases/mozilla-aurora/rev/a97a3d63fbed

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/d3e14f59b635
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/a97a3d63fbed

Comment 33

[Teodora Vermesan (:TeoVermesan)]

I reproduced the issue on 14-11 Aurora build using Nexus 6 (Android 5.1.1): the URL bar is not highlighted when long-tapping on it.
Verified as fixed on latest Nightly 45.0a1 (2015-11-24) and Aurora 44.0a2 (2015-11-25)