After thinking through some possibilities to avoid having to cancel editing mode when doing these things, we have to revert basically all of the editing mode state so it only really makes sense to cancel (and thus do these things). In a future iteration (or this one, if we can restore typed state - see bug 1085596 for possibilities on that), we might want to consider an editing mode per tab. Note: as decided by :antlam and I over IRC, we should cancel the forward and back toolbar buttons in editing mode (bug 1086980 is to revise this behavior in future iterations).

Comment on attachment 8509051 [details] [diff] [review] Part 2: Disable navigation buttons in editing mode Review of attachment 8509051 [details] [diff] [review]: ----------------------------------------------------------------- What's the rationale behind this? On desktop, for example, the back/forward buttons are kept active while editing.

Comment on attachment 8509051 [details] [diff] [review] Part 2: Disable navigation buttons in editing mode There is a bug with forward button state here.

(In reply to Lucas Rocha (:lucasr) from comment #3) > What's the rationale behind this? On desktop, for example, the back/forward > buttons are kept active while editing. about:home doesn't pop up when editing on desktop, which complicates the behavior of these buttons.

Saw some strange behavior when testing the patch for part 2. See bug 1087084.

Comment on attachment 8509125 [details] [diff] [review] Part 2: Disable navigation buttons in editing mode This code is still being wacky. :\

Got the code to be less janky and in the process, made the state more obvious (and thus consistent).

Comment on attachment 8510551 [details] [diff] [review] Part 2: Disable navigation buttons in editing mode Review of attachment 8510551 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Just a bit confused about that alpha tweaking in stopEditing(). ::: mobile/android/base/toolbar/BrowserToolbarNewTablet.java @@ +185,5 @@ > + protected String stopEditing() { > + // Undo the alpha change in startEditing's setButtonEnabled. > + final Drawable drawable = forwardButton.getDrawable(); > + if (drawable != null) { > + drawable.setAlpha(255); Why can't you just use setButtonEnabled() here? ::: mobile/android/base/toolbar/BrowserToolbarTabletBase.java @@ +113,5 @@ > + isForwardEnabled ? ForwardButtonAnimation.SHOW : ForwardButtonAnimation.HIDE); > + } > + } else { > + // I don't know the implications of changing this code on old tablet > + // (and no one is going to thoroughly test it) so duplicate the code. Fair enough.

(In reply to Lucas Rocha (:lucasr) from comment #11) > ::: mobile/android/base/toolbar/BrowserToolbarNewTablet.java > @@ +185,5 @@ > > + if (drawable != null) { > > + drawable.setAlpha(255); > > Why can't you just use setButtonEnabled() here? I guess I can, but I had to change it around a bit first.

Comment on attachment 8511258 [details] [diff] [review] Part 2.5: Use setButtonEnabled in stopEditing Review of attachment 8511258 [details] [diff] [review]: ----------------------------------------------------------------- Ok.

If a popup opens, we automatically switch to that tab now on new tablet (but not old tablet/phone) - basically undoing the targetTabForEditingMode hack. Since you have to hit the doorhanger first and the doorhanger doesn't appear without closing editing mode, this is a small use case that can be fixed when we re-do editing mode, where I think each page should have its own editing mode state. I'll tackle the tabs panel button change in the next part.

This works for the reload button too, but not the tabs button - that one is complicated because we can choose to leave editing mode open until a state change occurs (e.g. tab added/selected).

https://hg.mozilla.org/integration/fx-team/rev/8d47ea8c91f3 https://hg.mozilla.org/integration/fx-team/rev/a68a5f6029ff

Landed parts 1 & 2 (folded), just in case we enable the new tablet layout as default.

Comment on attachment 8513117 [details] [diff] [review] Part 4: Cancel editing mode in new tablet when new tabs are selected Review of attachment 8513117 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Any visual glitches that we need to address in follow-ups?

Comment on attachment 8513120 [details] [diff] [review] Part 3: Cancel editing mode when hitting toolbar buttons Review of attachment 8513120 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: mobile/android/base/BrowserApp.java @@ +2720,5 @@ > // the frequency of use for various actions. > Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.MENU, getResources().getResourceEntryName(itemId)); > > + if (NewTabletUI.isEnabled(this)) { > + cancelEditingMode(); Any reason for now just calling mBrowserToolbar.cancelEdit() here? IIRC, cancelEdit() already does this isEditing() check in stopEditing() under the hood.

https://hg.mozilla.org/mozilla-central/rev/8d47ea8c91f3 https://hg.mozilla.org/mozilla-central/rev/a68a5f6029ff

(In reply to Lucas Rocha (:lucasr) from comment #19) > Looks good. Any visual glitches that we need to address in follow-ups? * bug 1091306: There is some page flicker when cancelling editing mode to arrive at the same page you were just on (e.g. "Request desktop site", but not "new tab") * When entering editing mode (at all), the transition is not smooth, but this doesn't seem to be caused by my patches, at least parts 3 or 4. * I remember seeing some graphical glitches underneath the action bar menu while it's closing (e.g. an option is selected), but I don't see them any more

(In reply to Lucas Rocha (:lucasr) from comment #20) > > + if (NewTabletUI.isEnabled(this)) { > > + cancelEditingMode(); > > Any reason for now just calling mBrowserToolbar.cancelEdit() here? IIRC, > cancelEdit() already does this isEditing() check in stopEditing() under the > hood. Maybe a desire for bug 998000 to come back. :P It bothers me that there's a BrowserApp.enterEditingMode and a BrowserApp.commitEditingMode, but no BrowserApp.cancelEditingMode. If we rely on BrowserToolbar.cancelEdit(), note that we have an extraneous Telemetry.stopUISession call in cancelEdit (should we put it in an if?) before we return in BrowserToolbar.stopEditing, but that appears to have no effect [1] other than wasted CPU cycles. I'll use cancelEdit() for now, and we can create cancelEditingMode in bug 998000 if we think it's important. [1]: https://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/UITelemetry.jsm?rev=e63a1fb3757e#144

Uses cancelEdit instead.

Uses cancelEdit instead. ALSO, in onTabChanged, only calls cancelEdit when the SELECTED tab event is received, rather than SELECTED or ON_LOCATION_CHANGE - the latter caused problems when entering editing mode when the page was loading (where editing mode would then get canceled). Small, sensical change (especially because this has always been about tab selection, not location changes) so not re-requesting review.

Comment on attachment 8513928 [details] [diff] [review] Part 4: Cancel editing mode in new tablet when new tabs are selected Oops, habit.

Currently, editing mode appears to stop when the tabs panel button is hit (though, under the hood, we actually close it later). I'm thinking we land this as a v1 and fix up the back button behavior in bug 1086980.

Rewrote the patch to fix a few edge case bugs.

Anthony, I made a build for these patches [1]. I cancel editing mode when: * Reload is tapped * A menu item is tapped after the menu has been opened * The tabs panel is opened * A tab is switched (included added tabs) Back and forward are cancelled during this state. What do you think? Note that we may refine back button behavior in bug 1086980 (for example, to leave editing mode). [1]: https://people.mozilla.com/~mcomella/apks/mcomella-1086981_01.apk

Works fine for me except hitting the 3-dot overflow menu doesn't close the keyboard for me right now. I'm using: Nexus 7, horizontal orientation.

(In reply to Anthony Lam (:antlam) from comment #30) > Works fine for me except hitting the 3-dot overflow menu doesn't close the > keyboard for me right now. Good call - this patch should take care of that.

https://hg.mozilla.org/integration/fx-team/rev/1f830544ad5e https://hg.mozilla.org/integration/fx-team/rev/018d3b2ba7bf

https://hg.mozilla.org/mozilla-central/rev/1f830544ad5e https://hg.mozilla.org/mozilla-central/rev/018d3b2ba7bf

Comment on attachment 8514712 [details] [diff] [review] Part 6: Drop keyboard when clicking the menu button Review of attachment 8514712 [details] [diff] [review]: ----------------------------------------------------------------- Good.

I wonder if we should just cancel editing mode whenever the text entry loses focus?

(In reply to Lucas Rocha (:lucasr) from comment #35) > I wonder if we should just cancel editing mode whenever the text entry loses > focus? Cancelling editing mode means hiding about:home and since we cancel text entry focus (to drop the keyboard) when swiping between pages of the HomePager, as far as I know, we can't do that.

https://hg.mozilla.org/integration/fx-team/rev/17033c6fae5b https://hg.mozilla.org/integration/fx-team/rev/ad0e861aede4

https://hg.mozilla.org/mozilla-central/rev/17033c6fae5b https://hg.mozilla.org/mozilla-central/rev/ad0e861aede4

The edit mode in canceled when: Refresh button is tapped Tabs panel button is tapped A tab is switched Custom menu is opened, so I will mark this as Verified fixed. Builds: Firefox for Android 37.0a1 (2015-01-07) Firefox for Android 36.0a2 (2015-01-07) Device: Asus Transformer Pad TF300T (Android 4.2.1)