Closed
Bug 1086981
Opened 10 years ago
Closed 10 years ago
Cancel editing mode when pressing menu items
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 36
People
(Reporter: mcomella, Assigned: mcomella)
References
Details
Attachments
(8 files, 5 obsolete files)
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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).
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8509037 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8509051 -
Flags: review?(lucasr.at.mozilla)
Updated•10 years ago
|
Attachment #8509037 -
Flags: review?(lucasr.at.mozilla) → review+
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
(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.
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8509125 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8509051 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
Saw some strange behavior when testing the patch for part 2. See bug 1087084.
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8509125 -
Attachment is obsolete: true
Comment 11•10 years ago
|
||
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+
Assignee | ||
Comment 12•10 years ago
|
||
(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 13•10 years ago
|
||
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+
Assignee | ||
Comment 14•10 years ago
|
||
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)
Assignee | ||
Comment 15•10 years ago
|
||
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)
Assignee | ||
Comment 16•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8510551 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8510551 -
Attachment is obsolete: false
Assignee | ||
Comment 17•10 years ago
|
||
Assignee | ||
Comment 18•10 years ago
|
||
Landed parts 1 & 2 (folded), just in case we enable the new tablet layout as default.
Whiteboard: [leave-open]
Comment 19•10 years ago
|
||
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 20•10 years ago
|
||
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+
Comment 21•10 years ago
|
||
Assignee | ||
Comment 22•10 years ago
|
||
(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
Assignee | ||
Comment 23•10 years ago
|
||
(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
Assignee | ||
Comment 24•10 years ago
|
||
Uses cancelEdit instead.
Attachment #8513925 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8513120 -
Attachment is obsolete: true
Assignee | ||
Comment 25•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8513117 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8513925 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8513925 -
Flags: review+
Assignee | ||
Comment 26•10 years ago
|
||
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+
Assignee | ||
Comment 27•10 years ago
|
||
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)
Assignee | ||
Comment 28•10 years ago
|
||
Rewrote the patch to fix a few edge case bugs.
Attachment #8514675 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 29•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Attachment #8514590 -
Attachment is obsolete: true
Attachment #8514590 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(alam)
Comment 30•10 years ago
|
||
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)
Assignee | ||
Comment 31•10 years ago
|
||
(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)
Assignee | ||
Comment 32•10 years ago
|
||
Comment 33•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Blocks: new-tablet-v1
Updated•10 years ago
|
Attachment #8514675 -
Flags: review?(lucasr.at.mozilla) → review+
Comment 34•10 years ago
|
||
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+
Comment 35•10 years ago
|
||
I wonder if we should just cancel editing mode whenever the text entry loses focus?
Assignee | ||
Comment 36•10 years ago
|
||
(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.
Assignee | ||
Comment 37•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Whiteboard: [leave-open]
https://hg.mozilla.org/mozilla-central/rev/17033c6fae5b
https://hg.mozilla.org/mozilla-central/rev/ad0e861aede4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Comment 40•10 years ago
|
||
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
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•