1376147 - Tab history unavailable on ^z user interaction in awesomebar

Status: RESOLVED WORKSFORME

(Firefox :: Address Bar, defect)

Description

[rctgamer3]

Regression caused by 1358025 (likely part 4) - Tab history is not available on ^Z. I'm not sure if I should or might not be able to ^Z into a different tab's history, but I can't even undo/^Z to pages in the same website domain anymore.

Mozregression:
INFO: Last good revision: 92c5898618c21d80d8cccd49bc08ec5fc8924a45
INFO: First bad revision: 029d3143f52d17a218cd7dcb33ec3003fe5a8d9d
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=92c5898618c21d80d8cccd49bc08ec5fc8924a45&tochange=029d3143f52d17a218cd7dcb33ec3003fe5a8d9d

Comment 1

[rctgamer3]

Just to clarify, ^Z as in CTRL-Z / undo

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to rctgamer3 from comment #0)
> Regression caused by 1358025 (likely part 4) - Tab history is not available
> on ^Z. I'm not sure if I should or might not be able to ^Z into a different
> tab's history, but I can't even undo/^Z to pages in the same website domain
> anymore.

It should not be possible anymore to undo changes from a different tab.
do you see a problem while staying in the current tab? (I'm not sure what you mean by "I can't even undo/^Z to pages in the same website domain anymore", if it's in the same tab or in a different one.

Comment 3

[rctgamer3]

(In reply to Marco Bonardo [::mak] from comment #2)
> It should not be possible anymore to undo changes from a different tab.
> do you see a problem while staying in the current tab? (I'm not sure what
> you mean by "I can't even undo/^Z to pages in the same website domain
> anymore", if it's in the same tab or in a different one.

Undo/history is unavailable while staying in the current tab.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Could you please provide Step to reproduce the issue in comment 3? Thank you.

Comment 5

[Mike Taylor [:miketaylr]]

Assuming these are the STR, I cannot reproduce on mac or win10, in latest 55 beta:

1) in awesomebar, start to type google.com
2) type cmd+z to see history of related domains in awesomebar

Comment 6

[rctgamer3]

STR:

1) Type: "foobar" (without quotation marks)
2) Erase everything
3) Type: "barfoo"
4) Hit ^Z
5) foobar should show up when ^Z is performed, but it only deletes the word until the first character, e.g. only "b" shows up instead of the previously entered value.

STR #2 (similar):
1) Navigate to mozilla.org
2) Navigate to publicsuffix.org
2) Naviate to mozillians.org
3) Hit ^Z
4) History, publicsuffix.org and mozilla.org do not show up when pressing ^Z. No history at all shows up.

This worked fine before bug 1358025 was "fixed".

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Makoto-san, what do you think about this? Sounds different from bug 1369592, since it doesn't involve different tabs.

Comment 8

[Makoto Kato [:m_kato]]

(In reply to rctgamer3 from comment #6)
> STR #2 (similar):
> 1) Navigate to mozilla.org
> 2) Navigate to publicsuffix.org
> 2) Naviate to mozillians.org
> 3) Hit ^Z
> 4) History, publicsuffix.org and mozilla.org do not show up when pressing
> ^Z. No history at all shows up.

Since awesome bar doesn't use setUserInput, it is correct as textbox.  Since STR1 seems to be another issue, I will look it.

Comment 9

[Makoto Kato [:m_kato]]

STR #1 should be undo transaction.  autocomplete.xml doesn't use setUserInput for value setter.  So we should use it instead.

Comment 10

[Makoto Kato [:m_kato]]

Attachment: Bug 1376147 - Part 1. autocomplete widget should use setUserInput to allow undo transaction.

By bug 1358025, input.value setter doesn't create undo transaction. Its behavior is same as all browsers.

But, since autocomplete widget that uses on url bar uses input.value instead of setUserInput, auto completion doesn't allow undo transaction.  So for better user interaction, we should allow undo transaction on urlbar.

Review commit: https://reviewboard.mozilla.org/r/154440/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154440/

Comment 11

[Makoto Kato [:m_kato]]

Attachment: Bug 1376147 - Part 2. Add mochitest.

Review commit: https://reviewboard.mozilla.org/r/154442/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154442/

Comment 12

[Makoto Kato [:m_kato]]

after updating test, I will resend review

Comment 13

[Makoto Kato [:m_kato]]

Humm, since setUserInput moves focus and posts input event, some doorhanger tests are failure.

Comment 14

[Mike Taylor [:miketaylr]]

Makoto, any updates? Thanks!

Comment 15

[Makoto Kato [:m_kato]]

according to comment #6, reporter reports 2 issues,

(In reply to rctgamer3 from comment #6)
> STR:
> 
> 1) Type: "foobar" (without quotation marks)
> 2) Erase everything
> 3) Type: "barfoo"
> 4) Hit ^Z
> 5) foobar should show up when ^Z is performed, but it only deletes the word
> until the first character, e.g. only "b" shows up instead of the previously
> entered value.

patch is coming for this.


> STR #2 (similar):
> 1) Navigate to mozilla.org
> 2) Navigate to publicsuffix.org
> 2) Naviate to mozillians.org
> 3) Hit ^Z
> 4) History, publicsuffix.org and mozilla.org do not show up when pressing
> ^Z. No history at all shows up.

:mak, is this behavior is expected as product design?  Although I test on all major browsers (Safari, Chrome, and IE), undo operation doesn't back to previous address.  So current behavior is same as all major browsers.  All browser's text box doesn't create undo transaction by input.value setter.

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Makoto Kato [:m_kato] from comment #15)
> > STR #2 (similar):
> > 1) Navigate to mozilla.org
> > 2) Navigate to publicsuffix.org
> > 2) Naviate to mozillians.org
> > 3) Hit ^Z
> > 4) History, publicsuffix.org and mozilla.org do not show up when pressing
> > ^Z. No history at all shows up.
> 
> :mak, is this behavior is expected as product design?  Although I test on
> all major browsers (Safari, Chrome, and IE), undo operation doesn't back to
> previous address.

Maybe it's platform dependent? I tested Chrome and Edge on Windows 10, and both on ^Z suggested me the previous urls (obviously as far as I do all the load in the same tab). I just did paste+Enter of each of the above urls in a tab and then ^z enough times.

I honestly don't have enough data to tell if this behavior is expected by users or not, but evidence seems to be that we don't behave like the other major browsers.

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

In the end, I think we should try to be coherent with the other browsers, in lack of better data to evaluate the case.

Comment 18

[Makoto Kato [:m_kato]]

(In reply to Marco Bonardo [::mak] from comment #16)
> Maybe it's platform dependent? I tested Chrome and Edge on Windows 10, and
> both on ^Z suggested me the previous urls (obviously as far as I do all the
> load in the same tab). I just did paste+Enter of each of the above urls in a
> tab and then ^z enough times.

I test on both macOS and Widnows.

STR on Chrome for Windows
1. type url: https://www.mozilla.org/en-US/ and [Enter]
2. Click "Firefox" link, then navigate to https://www.mozilla.org/en_US/firefox
3. Click "Internet Health" link, then navigate to https://www.mozilla.org/en-US/internet-health/
4. Ctrl+Z

Result:
url bar is https://www.mozilla.org

When same steps on Firefox 54, url bar is https://www.mozilla.org/en_US/firefox.  It isn't same behavior on each browser.

Comment 19

[Makoto Kato [:m_kato]]

Of course, maybe, we can be back to same behavior (always turning on undo stack) on chrome process (but I don't know this is acceptable).  If we move from XUL to HTML for UI, we will hit same issue.  So I want to know correct design for address bar.

Comment 20

[Makoto Kato [:m_kato]]

(In reply to Makoto Kato [:m_kato] from comment #18)
> Result:
> url bar is https://www.mozilla.org

correct is https://www.mozilla.org/en-US/

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Makoto Kato [:m_kato] from comment #18)
> STR on Chrome for Windows
> 1. type url: https://www.mozilla.org/en-US/ and [Enter]
> 2. Click "Firefox" link, then navigate to
> https://www.mozilla.org/en_US/firefox
> 3. Click "Internet Health" link, then navigate to
> https://www.mozilla.org/en-US/internet-health/
> 4. Ctrl+Z

Ok, what I tested was pasting and visiting 3 unrelated urls as suggested in comment 6, rather than browsing through links.
I tried your steps in Edge, it's bogus. It seems to go back if I press CTRL+Z many times, but then it can't redo properly. It's just bogus. Chrome doesn't do anything.
I don't think these specific steps are a big deal, it's not urls the user typed, so we likely don't care to provide undo here, I'm fine with it being broken.

I think it's more important the case where the user types values. In the same tab:

1) paste/type mozilla.org into the urlbar, press Enter
2) paste/type publicsuffix.org, press Enter
3) paste/type mozillians.org, press Enter
4) Hit ^Z
5) History, publicsuffix.org and mozilla.org do not show up when pressing ^Z.

These values have been typed by the user, and as such ^Z *should* probably cycle through them, and both Edge and Chrome cycle through them afaict.

This is a bit "bogus" in all the browsers, so I don't think it has a critical priority.
If there's an easy way to at least cycle through stuff the user typed/pasted, it would likely be preferred.

Comment 22

[Makoto Kato [:m_kato]]

Comment on attachment 8883519 [details]
Bug 1376147 - Part 1. autocomplete widget should use setUserInput to allow undo transaction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154440/diff/1-2/

Comment 23

[Makoto Kato [:m_kato]]

Comment on attachment 8883520 [details]
Bug 1376147 - Part 2. Add mochitest.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154442/diff/1-2/

Comment 24

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Could you please clarify which of the STR in comment 6 this is handling? Is it instead handling comment 5?
By looking at the patch it seems like it's fixing a totally different bug with autoFill, that is not reported in this bug. that sounds like something to fix, but I cannot relate the reported failures with the patch.

Looks like we have 3 issues reported here, 1 in comment 5 and 2 in comment 6, and the patch is just handling comment 5? I guess we may want to handle the cases in dependent bugs, one per issue, to simplify tracking.

Regarding the patch, when the controller does autofill, it invokes setTextValueWithReason with aReason == Components.interfaces.nsIAutoCompleteInput.TEXTVALUE_REASON_COMPLETEDEFAULT. The textValue setter invokes the value setter, so we could set a temporary property to know that the value setter has been invoked as a consequence of an autoFill, without having to guess through .focused (provided that is the scope).

Comment 25

[Makoto Kato [:m_kato]]

(In reply to Marco Bonardo [::mak] from comment #24)
> Could you please clarify which of the STR in comment 6 this is handling? Is
> it instead handling comment 5?
> By looking at the patch it seems like it's fixing a totally different bug
> with autoFill, that is not reported in this bug. that sounds like something
> to fix, but I cannot relate the reported failures with the patch.

This fixes part 1. case only.  Since setUserInput moves focus, this method won't help STR #2's issue (like comment #18)

> Looks like we have 3 issues reported here, 1 in comment 5 and 2 in comment
> 6, and the patch is just handling comment 5? I guess we may want to handle
> the cases in dependent bugs, one per issue, to simplify tracking.

OK, I will separate bugs.

> Regarding the patch, when the controller does autofill, it invokes
> setTextValueWithReason with aReason ==
> Components.interfaces.nsIAutoCompleteInput.TEXTVALUE_REASON_COMPLETEDEFAULT.
> The textValue setter invokes the value setter, so we could set a temporary
> property to know that the value setter has been invoked as a consequence of
> an autoFill, without having to guess through .focused (provided that is the
> scope).

OK, I want to detect auto fill.  So I should use it if possible.

Also, value setter is used on navigating URL (to update url address on address bar).  But this setter to update current display URL doesn't want focus change, so I cannot use setUserInput for all situation.

Comment 26

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8883519 [details]
Bug 1376147 - Part 1. autocomplete widget should use setUserInput to allow undo transaction.

Clearing review, I'd prefer this to be split across dependency bugs, and I gave some suggestions already.
In case I'm away, you can ask for review to :adw

Note that yesterday I've seen a discussion in #developers between Masayuki and Ehsan about the possibility to restore old edit history behavior for chrome context, so this may maybe become obsolete if that happens...

Comment 27

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

in particular, I think they were discussing the fix in bug 1386222 (I don't know the editor code, so you can tell better than me)

Comment 28

[Mike Taylor [:miketaylr]]

Marking as 56 fix-optional, given Comment #26. We may want to do the same for 57...

Comment 29

[David Bolter [:davidb] (NeedInfo me for attention)]

I can't recreate this bug on nightly (58) haven't tried others...

Comment 30

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I think bug 1386222 has pretty much restored the behavior for xul as it was before, so as far as we don't move to html this bug may not reproduce.

Comment 31

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

P5 until we understand if this is still an issue, could be WFM.

Comment 32

[Marcia Knous [:marcia]]

ni on Andrei for QA to try to reproduce.

Comment 33

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

P5 would indicate that we're not working on a patch, but would accept one, so unsetting priority as this bug is still being triaged. If you're unable to get a reproduction, don't be shy about resolving as WFM.

Comment 34

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Yeah, for now I'd resolve to WFM, because it looks like recent changes have reverted the behavior.
Though, the behavior is not crystal clear and we may still have some edge-cases not working very well. We'll look into those bugs when identified and filed.