1236364 - address bar pull down delete deletes wrong entry

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Marc Auslander]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20151228134903

Steps to reproduce:

type a search word into the address bar and get a list of url's containing that word.
hover over an entry to select it.  press delete


Actual results:

the url below the selected entry disappears and has been deleted (I think)


Expected results:

the selected entry should be deleted.

Comment 1

[Marc Auslander]

In looking more carefully, I think the correct entry is deleted but it does not disappear from the list - instead the one below disappears but was not in fact deleted.

Comment 2

[Loic]

Are you sure it's not the 1st entry which is deleted but doesn't disappear from the drop-down list?

If yes, it's a dupe of bug 1234258.

Comment 3

[Marc Auslander]

Yes but 1234258 is incomplete - I'll comment there.  thanks.

Comment 4

[Drew Willcoxon :adw]

From bug 1234258:

(In reply to Marc Auslander from bug 1234258 comment #23)
> I just tested
> https://hg.mozilla.org/mozilla-central/rev/
> e790bba372f14241addda469a4bdb7ab00786ab3
> 
> My bug 1236364 which was duped to this bug is not fixed in that build.
> 
> What I see is that after getting a list of potential matches from history by
> typing a match phrase in the address bar, if I hover over any entry,
> including the first, and press delete, the hovered over entry is still
> visible and the subsequent entry disappears.  But if I then carefully do NOT
> try to visit anything in the list but start again, I find that the hovered
> over entry is in fact gone from history after all!
> 
> Note that this appears, when hovering over the first entry, as the bug
> described here, but it isn't - the first entry is deleted but the list is
> incorrectly updated.  Of course if you then visit that entry, it is
> re-entered so you might think it was not deleted.

Comment 5

[Drew Willcoxon :adw]

Guessing this is related to bug 1192505 or the other work we did to stop updating results once the mouse hovers over the popup.  I'll look at this tomorrow if you don't take it first, Marco.

Comment 6

[Loic]

Attachment: Recording #8.mp4

Yes, the deletion of entries in the history drop-down list is totally broken.
The hovered entry is not deleted but the entry below, and the location bar displays the URL of the wrong deleted entry...

Comment 7

[Marc Auslander]

I see something different. In my testing:

The hovered entry IS deleted from history, but not from the drop-down list.  The entry following is deleted from the drop-down list but NOT from history.

What I do is press delete and then abandon that test all together by clearing the awesome bar.  Then I do the test again by typing the search term again.

Comment 8

[Drew Willcoxon :adw]

Attachment: patch

This works, but maybe it's too hacky?

Comment 9

[Drew Willcoxon :adw]

That patch fixes this specific bug BTW, but I think there are still other problems related to deleting from the urlbar.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8709802 [details] [diff] [review]
patch

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

If we need to go this way, I'd rather change nsIAutoCompletePopup::invalidate to take an aReason param. All consumers are very likely javascript and they will ignore the param and keep working unless they get updated to handle it.

::: toolkit/content/widgets/autocomplete.xml
@@ +1246,4 @@
>                if (item.getAttribute("text") == trimmedSearchString &&
>                     (item.getAttribute("url") == url ||
> +                    this.richlistbox.mouseSelectedIndex === this._currentIndex) &&
> +                    !this.input._handlingDelete

I'm not sure I understand the reason we need this. Here we reuse the item only if its text and url attributes are exactly the same.
Now, I understand "text", but why does controller.getValueAt return the same url we just removed with a call to result->RemoveValueAt()?

Comment 11

[Drew Willcoxon :adw]

Attachment: patch v2

(In reply to Marco Bonardo [::mak] from comment #10)
> Now, I understand "text", but why does controller.getValueAt return the same
> url we just removed with a call to result->RemoveValueAt()?

It doesn't, you're right, but if you look at the conditional:

  if (item.getAttribute("text") == trimmedSearchString &&
       (item.getAttribute("url") == url ||
        this.richlistbox.mouseSelectedIndex === this._currentIndex)
      ) {

you'll see that it's "the URLs are the same *OR* the current index is moused over."  It took me a while to spot that too when I was trying to answer your question.  So the scenario is that you mouse over a result, you hit Delete, and even though the URLs are different, the conditional is true because of that second term in the ||.

So I still think the way the previous patch works is basically the way to fix this.  This new patch uses an invalidation reason like you suggest.  But let me know if you can think of something better.

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8711266 [details] [diff] [review]
patch v2

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

::: toolkit/content/widgets/autocomplete.xml
@@ +1248,5 @@
> +                  ((invalidateReason == iface.INVALIDATE_REASON_NEW_RESULT &&
> +                    (item.getAttribute("url") == url ||
> +                     this.richlistbox.mouseSelectedIndex === this._currentIndex)) ||
> +                   (invalidateReason == iface.INVALIDATE_REASON_DELETE &&
> +                    item.getAttribute("url") == url))) {

I think there's no much value into checking url for INVALIDATE_REASON_DELETE.
The reason to completely reuse a richlistitem is performance, but in the DELETE case we are already in an edge case, plus the case the search returned 2 identical uris and the user removed one is even less likely and uninteresting.
We are not winning much, not worth the complication.

I'd say you could simplify the if by making so that we try to completely reuse the richlistitem only in the new result case. In the delete case we will keep reusing the item, but not completely.

(pseudo-code)
if (text &&
    reason_new_result &&
    (url ||
     mouseSelectedIndex))

Comment 13

[Drew Willcoxon :adw]

Yeah, fair enough.

Comment 14

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/fx-team/rev/1508986bb0cb

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1508986bb0cb

Comment 17

[Khalid Syfullah Zaman [:khalid32]]

I have reproduced this bug on Nightly 46.0a1 (2016-01-03) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Nightly 47.0a1!

Build ID: 20160127030236
User Agent: Mozilla/5.0 (X11; Linux i686; rv:47.0) Gecko/20100101 Firefox/47.0

Comment 18

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox Nightly 46.0a1 (Build ID: 20160103030302) on 
windows 8.1 64-bit with the instructions from comment 0.

Verified as fixed with latest Firefox Nightly 47.0a1 (Build ID: 20160201030241)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0

As this bug is also verified on ubuntu (Comment 17), I am marking this as verified!

Comment 20

[Marco Bonardo [:mak]]

Can we uplift this?

Comment 21

[YF (Yang)]

[Tracking Requested - why for this release]:
a regression.

Comment 22

[Drew Willcoxon :adw]

Attachment: landed patch

Approval Request Comment
[Feature/regressing bug #]: bug 1192505, recent UnifiedComplete and urlbar work
[User impact if declined]: Deleting urlbar popup results while the mouse is hovered over them will appear to be broken
[Describe test coverage new/current, TreeHerder]: No new test for this bug -- actually there is probably no automated test coverage for this specific problem, but there is coverage for deleting results in general
[Risks and why]: Low risk, small code change
[String/UUID change made/needed]: None

Comment 23

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #22)
> [String/UUID change made/needed]: None

Actually there is a uuid change to nsIAutoCompletePopup.

Comment 24

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #23)
> (In reply to Drew Willcoxon :adw from comment #22)
> > [String/UUID change made/needed]: None
> 
> Actually there is a uuid change to nsIAutoCompletePopup.

yep, but it's backwards compatible, should not be a problem.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8717048 [details] [diff] [review]
landed patch

Fix a regression, taking it.
Should be in 45 beta 5.

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ef6572f4cd24

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/5a12a3f9b5e9

Comment 29

[Rezaul Huque Nayeem]

Reproduced the bug in firefox nightly 46.0a1(2016-01-03) with windows 10 (64 bit)

Verified as fixed with latest firefox beta 45.0b9 (Build ID: 20160223142613)
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:45.0) Gecko/20100101 Firefox/45.0

and latest firefox aurora 46.0a2 (Build ID: 20160302004006)
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:46.0) Gecko/20100101 Firefox/46.0

Comment 32

[Tummala Dhanvi [:c0mrad3][UTC+5.30]]

(In reply to Rezaul Huque Nayeem from comment #29)
> Reproduced the bug in firefox nightly 46.0a1(2016-01-03) with windows 10 (64
> bit)
> 
> Verified as fixed with latest firefox beta 45.0b9 (Build ID: 20160223142613)
> Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:45.0) Gecko/20100101
> Firefox/45.0
> 
> and latest firefox aurora 46.0a2 (Build ID: 20160302004006)
> Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:46.0) Gecko/20100101
> Firefox/46.0

So I am changing the status of these two versions as verified

Comment 33

[Tummala Dhanvi [:c0mrad3][UTC+5.30]]

Verified as Fixed in Firefox ESR 45 Debian 8 GNU/Linux 

now the entry below the selected one is deleted when the delete button is pressed in the Firefox ESR 45