Some ideas: * The hack: we can store the text as a variable and restore it when the tab is switched back to * Better, but perhaps v2?: Have an editing mode per tab and we technically don't have to "restore" state - editing mode is essentially its own page

Part 2 will be to restore the editing mode state nearly exactly (browser search, editing text, and selection).

Restores all of the editing mode state now, folded into the original part 1. Still to go: restored text includes the auto-complete so removing that. Note that I wasn't sure where to put TabEditingState.

Comment on attachment 8518211 [details] [diff] [review] Part 1: Restore editing text when switching between tabs in editing mode Review of attachment 8518211 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/BrowserApp.java @@ +235,5 @@ > // The tab to be selected on editing mode exit. > private Integer mTargetTabForEditingMode; > > + // The edited text in the toolbar from the most recent tab to be unselected. > + private String unselectedTabEditingText; mUnselectedTabEditingText for consistency. @@ +332,5 @@ > + > + super.onTabChanged(tab, msg, data); > + } > + > + private void updateEditingMode(final Tab selectedTab) { updateEditingModeForTab? ::: mobile/android/base/toolbar/BrowserToolbar.java @@ +803,5 @@ > */ > public String cancelEdit() { > + final Tab selectedTab = Tabs.getInstance().getSelectedTab(); > + if (selectedTab != null) { > + selectedTab.setIsEditing(false); Instead of update the 'isEditing' tab state in different spots, maybe it should all be done in BrowserApp's stop/start editing listeners.

Comment on attachment 8518249 [details] [diff] [review] Part 1: Restore editing text when switching between tabs in editing mode Review of attachment 8518249 [details] [diff] [review]: ----------------------------------------------------------------- Looking good with the stop/start listener changes. ::: mobile/android/base/BrowserApp.java @@ +333,5 @@ > + > + super.onTabChanged(tab, msg, data); > + } > + > + private void updateEditingMode(final Tab selectedTab) { updateEditingModeForTab? @@ +342,5 @@ > + storeEditingState(mLastEditingState); > + > + if (selectedTab.isEditing()) { > + enterEditingMode(); > + restoreEditingState(selectedTab.mEditingState); Add a getter to the editing state? To avoid breaking encapsulation and all. @@ +348,5 @@ > mBrowserToolbar.cancelEdit(); > } > + } > + > + private void storeEditingState(final TabEditingState editingState) { saveEditingState? store/restore look confusingly similar :-) ::: mobile/android/base/toolbar/BrowserToolbar.java @@ +415,5 @@ > }); > } > } > > + public void storeEditingState(final TabEditingState editingState) { Maybe storeTabEditingState for clarity?

Updated for nits.

Well, that was easy.

Comment on attachment 8518271 [details] [diff] [review] Part 1: Restore editing text when switching between tabs in editing mode Review of attachment 8518271 [details] [diff] [review]: ----------------------------------------------------------------- Awesome.

Comment on attachment 8518298 [details] [diff] [review] Part 2: Do not restore autocomplete text from editing mode Review of attachment 8518298 [details] [diff] [review]: ----------------------------------------------------------------- Great.

https://hg.mozilla.org/integration/fx-team/rev/17dd668f5444 https://hg.mozilla.org/integration/fx-team/rev/3357e9e81614

sorry had to back this out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1129492&repo=fx-team

Seems to have hit hit my assertion that we have the selected tab when calling updateEditingModeFromTab(Tab): 06:31:04 WARNING - PROCESS-CRASH | java-exception | java.lang.IllegalStateException: Expected given tab to be selected at org.mozilla.gecko.BrowserApp.onTabChanged(BrowserApp.java:331) The issue is that `selectTab` is called from the gecko thread in `Tabs.handleMessage`, which updates `mSelectedTab`, then posts to the UI thread to notify `onTabChanged(TabEvents.SELECTED)` listeners. Before the listeners can run again (using that selected tab as their argument), `Tabs.handleMessage` can be called again, selecting a new tab, changing `mSelectedTab` directly, and invalidating the tab passed to the listeners.

Decided to log instead of throw; also rebase.

(In reply to Michael Comella (:mcomella) from comment #12) > Decided to log instead of throw; also rebase. Motivation from a comment in the patch: > (bug 1086983 comment 11) Because the tab may be selected from the gecko thread and we're > running this code on the UI thread, the selected tab argument may not still refer to the . selected tab. However, that means this code should be run again and the initial state > changes will be overridden. As an optimization, we can skip this update, but it may have > unknown side-effects so we don't.

https://hg.mozilla.org/integration/fx-team/rev/53d7570e20f2 https://hg.mozilla.org/integration/fx-team/rev/36e434d7d990

https://hg.mozilla.org/mozilla-central/rev/53d7570e20f2 https://hg.mozilla.org/mozilla-central/rev/36e434d7d990