Closed Bug 1086981 Opened 7 years ago Closed 7 years ago

Cancel editing mode when pressing menu items

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 36

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(8 files, 5 obsolete files)

1.05 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
9.88 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
2.66 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
9.18 KB, patch
Details | Diff | Splinter Review
1.42 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
3.16 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
2.11 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
1.11 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
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).
Attachment #8509037 - Flags: review?(lucasr.at.mozilla) → review+
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.
Attachment #8509051 - Flags: review?(lucasr.at.mozilla)
(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.
Attachment #8509051 - Attachment is obsolete: true
Saw some strange behavior when testing the patch for part 2. See bug 1087084.
Duplicate of this bug: 1087084
Comment on attachment 8509125 [details] [diff] [review]
Part 2: Disable navigation buttons in editing mode

This code is still being wacky. :\
Attachment #8509125 - Flags: review?(lucasr.at.mozilla)
Got the code to be less janky and in the process, made the state more obvious (and thus consistent).
Attachment #8510551 - Flags: review?(lucasr.at.mozilla)
Attachment #8509125 - Attachment is obsolete: true
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.
Attachment #8510551 - Flags: review?(lucasr.at.mozilla) → review+
(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.
Attachment #8511258 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8511258 [details] [diff] [review]
Part 2.5: Use setButtonEnabled in stopEditing

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

Ok.
Attachment #8511258 - Flags: review?(lucasr.at.mozilla) → review+
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.
Attachment #8513117 - Flags: review?(lucasr.at.mozilla)
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).
Attachment #8513120 - Flags: review?(lucasr.at.mozilla)
Attachment #8510551 - Attachment is obsolete: true
Attachment #8510551 - Attachment is obsolete: false
Landed parts 1 & 2 (folded), just in case we enable the new tablet layout as default.
Whiteboard: [leave-open]
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?
Attachment #8513117 - Flags: review?(lucasr.at.mozilla) → review+
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.
Attachment #8513120 - Flags: review?(lucasr.at.mozilla) → review+
(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.
Attachment #8513925 - Flags: review?(lucasr.at.mozilla)
Attachment #8513120 - Attachment is obsolete: true
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.
Attachment #8513928 - Flags: review?(lucasr.at.mozilla)
Attachment #8513117 - Attachment is obsolete: true
Attachment #8513925 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8513928 [details] [diff] [review]
Part 4: Cancel editing mode in new tablet when new tabs are selected

Oops, habit.
Attachment #8513928 - Flags: review?(lucasr.at.mozilla) → review+
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.
Attachment #8514590 - Flags: review?(lucasr.at.mozilla)
Rewrote the patch to fix a few edge case bugs.
Attachment #8514675 - Flags: review?(lucasr.at.mozilla)
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
Attachment #8514590 - Attachment is obsolete: true
Attachment #8514590 - Flags: review?(lucasr.at.mozilla)
Flags: needinfo?(alam)
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.
Flags: needinfo?(alam)
(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.
Attachment #8514712 - Flags: review?(lucasr.at.mozilla)
Attachment #8514675 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8514712 [details] [diff] [review]
Part 6: Drop keyboard when clicking the menu button

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

Good.
Attachment #8514712 - Flags: review?(lucasr.at.mozilla) → review+
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.
Whiteboard: [leave-open]
Duplicate of this bug: 1082863
https://hg.mozilla.org/mozilla-central/rev/17033c6fae5b
https://hg.mozilla.org/mozilla-central/rev/ad0e861aede4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
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)
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.