Closed Bug 1236364 Opened 4 years ago Closed 4 years ago

address bar pull down delete deletes wrong entry

Categories

(Firefox :: Address Bar, defect, P1)

44 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 47
Tracking Status
firefox45 --- verified
firefox46 --- verified
firefox47 --- verified
firefox-esr45 45+ verified

People

(Reporter: marcausl, Assigned: adw)

References

Details

(Keywords: regression, Whiteboard: [fxsearch][bugday-20160127])

Attachments

(3 files, 1 obsolete file)

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.
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.
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.
Component: Untriaged → Location Bar
Yes but 1234258 is incomplete - I'll comment there.  thanks.
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1234258
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.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: DUPLICATE → ---
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.
Status: REOPENED → NEW
Attached video 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...
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.
Priority: -- → P1
Whiteboard: [fxsearch]
Assignee: nobody → adw
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
This works, but maybe it's too hacky?
Attachment #8709802 - Flags: review?(mak77)
That patch fixes this specific bug BTW, but I think there are still other problems related to deleting from the urlbar.
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()?
Attachment #8709802 - Flags: review?(mak77)
Attached patch patch v2Splinter Review
(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.
Attachment #8709802 - Attachment is obsolete: true
Attachment #8711266 - Flags: review?(mak77)
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))
Attachment #8711266 - Flags: review?(mak77) → review+
Yeah, fair enough.
Blocks: 669129
Blocks: 1192505
Keywords: regression
Duplicate of this bug: 1242849
https://hg.mozilla.org/mozilla-central/rev/1508986bb0cb
Status: ASSIGNED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
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
QA Whiteboard: [bugday-20160127]
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!
Status: RESOLVED → VERIFIED
Whiteboard: [fxsearch] → [fxsearch][bugday-20160127]
Duplicate of this bug: 1246395
Can we uplift this?
Flags: needinfo?(adw)
[Tracking Requested - why for this release]:
a regression.
Attached patch landed patchSplinter Review
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
Flags: needinfo?(adw)
Attachment #8717048 - Flags: approval-mozilla-beta?
Attachment #8717048 - Flags: approval-mozilla-aurora?
(In reply to Drew Willcoxon :adw from comment #22)
> [String/UUID change made/needed]: None

Actually there is a uuid change to nsIAutoCompletePopup.
(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 on attachment 8717048 [details] [diff] [review]
landed patch

Fix a regression, taking it.
Should be in 45 beta 5.
Attachment #8717048 - Flags: approval-mozilla-beta?
Attachment #8717048 - Flags: approval-mozilla-beta+
Attachment #8717048 - Flags: approval-mozilla-aurora?
Attachment #8717048 - Flags: approval-mozilla-aurora+
QA Whiteboard: [bugday-20160127] → [good first verify][bugday-20160127]
Duplicate of this bug: 1250742
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
QA Whiteboard: [good first verify][bugday-20160127] → [good first verify][bugday-20160127][bugday-20160302]
Duplicate of this bug: 1253800
Duplicate of this bug: 1255000
(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
QA Whiteboard: [good first verify][bugday-20160127][bugday-20160302] → [good first verify][bugday-20160127][bugday-20160302][bugday-20160316]
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
You need to log in before you can comment on or make changes to this bug.