Closed Bug 1304617 Opened 8 years ago Closed 8 years ago

[Regression] Bookmark menu context menu item order is messed up after right-clicking a folder

Categories

(Firefox :: Bookmarks & History, defect, P2)

defect
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 52
Tracking Status
firefox49 --- wontfix
firefox-esr45 --- unaffected
firefox50 + verified
firefox51 --- verified
firefox52 --- verified

People

(Reporter: fireattack, Assigned: dao)

References

Details

(Keywords: regression)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.34 Safari/537.36

Steps to reproduce:

1. Click "Bookmark menu" icon, (Next to the "Bookmark star" icon.), or the bookmark menu on menu bar (press Alt then "Bookmarks")
2. Right-click a bookmark folder and note "Open All in Tabs" is at the top.
3. After that, right-click a normal bookmark, or right-click a bookmark folder again.


Actual results:

In step 3, every menu items, except "Hide recently bookmarked", are in the reverse order now.


Expected results:

They shouldn't.

This is a regression caused by the patch of bug 1248268 (tested with Mozregression).
Component: Untriaged → Bookmarks & History
Blocks: 1248268
Keywords: regression
I couldn't reproduce at the beginning, until I right clicked one of the recent bookmarks
btw, to me doesn't look like they are all inverted, it's just Open All in Tabs that shifts to the bottom?
Priority: -- → P2
>I couldn't reproduce at the beginning, until I right clicked one of the recent bookmarks

I can reproduce by only

1) Right-click the default "Mozilla Firefox" folder
and then
2) Right-click any bookmark *in the root of menu*, including the recent bookmarks, to see that the menu has wrong order


>btw, to me doesn't look like they are all inverted, it's just Open All in Tabs that shifts to the bottom?


Yeah you're right, I was wrong. It's indeed just the first item ("Open all in tabs" or "Open") became the bottom one.
Loic, in bug 1305272, found the regression range to be at 
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=83a1b9c7ae8ddf0eebf6c5cbfebd9cfc5945dcf6&tochange=e8112fba80d64c7c6615c9019178e68f60b5d718

Mike de Boer — Bug 1248267 - allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions. r=mak,dao

So it's a race between us, Dão! :)
I just checked this locally and it's my patch that's causing the regression.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: qe-verify+
Flags: needinfo?(dao+bmo)
Flags: firefox-backlog+
Iteration: --- → 52.1 - Oct 3
Points: --- → 5
Blocks: 1248267
No longer blocks: 1248268
Reproducible on the "last good build" in pushlog above. Better ask Loic and double-check results.
This is regression from bug 1248268, NOT from bug 1248267. More info:



Firefox untriaged

Summary:
Right-clicking in bookmarks panel changes sequence of menuitems in context menu

Keywords:
regression

Blocks:
1248268


>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:
1. Open bookmarks panel
2. Right-click any normal bookmark
3. Right-click any normal folder
4. Repeat Step 2

AR:  Menuitems "Open" and "Open in a new tab" moved almost to the end of context menu
ER:  Sequence of menuitems shouldn't change


STR_2:
1. Open bookmarks panel
2. Right-click any normal bookmark
3. Right-click any normal folder
4. Repeat Step 2
5. Repeat Step 3
6. Repeat Step 2
7. Repeat Step 3

AR:  Context menu starts with separator. Some menuitems moved almost to the end of context menu
ER:  Sequence of menuitems shouldn't change

This is regression from bug 1248268. Regression range:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=70b3e6ac383941005652196f432d9134dea468a7&tochange=f98e3add979e3b2eba6461fdaed2cfc80961ff6f
Thanks for clearing things up Mike. I don't why mozregression told me it's 1248268 on my end.
(In reply to Benjamin Peng from comment #8)
> Thanks for clearing things up Mike. I don't why mozregression told me it's
> 1248268 on my end.

When we meet IRL, I'll slap you silly.
(In reply to Mike de Boer [:mikedeboer] from comment #9)
> (In reply to Benjamin Peng from comment #8)
> > Thanks for clearing things up Mike. I don't why mozregression told me it's
> > 1248268 on my end.
> 
> When we meet IRL, I'll slap you silly.

Sorry, what do you mean? I missed a word "know" in my previous comment.

What I meant is that when I was doing Mozregression, the result INDEED was bug 1248268 as the cause of regression, not bug 1248267. Unless there are bugs in Mozregression.

And it looks like arni2033 can repeat the same result as me.

Loic, could you double check?
Flags: needinfo?(epinal99-bugzilla2)
I didn't rerun Mozregression, as the other bug has been duped, I trust the STRs and reg range found here.
Flags: needinfo?(epinal99-bugzilla2)
(In reply to Benjamin Peng from comment #10)
> > When we meet IRL, I'll slap you silly.
> 
> Sorry, what do you mean? I missed a word "know" in my previous comment.

It was a joke! It's an absurdist exaggeration, by which I meant I'm not bothered at all. It also has a amicable connotation with which I attempt to express my gratitude for the work you're doing for the team.
In other words: thanks!
Tracked for Fx50 since it's a recent regression.
Iteration: 52.1 - Oct 3 → 52.2 - Oct 17
Oh dear, it seems like I might be causing a bit of confusion here, but it does seem more likely that bug 1248268 is causing this regression and _not_ bug 1248267...
I'll investigate this a bit more, but if I really can't find a fix soonish, I might have to push this over.
off-hand it sounds like a bug in the ordinal attribute management.
The menuitems are never removed or moved, they are just ordered using the "ordinal" attribute and that seems to be confusing the menu.
There are various bugs around ordinal, like bug 102054, I wonder if when we add the new elements with ordinal 2, the code automatically assigned an ordinal to the other elements, and that causes a dynamic update that hits that bug (moving the first entry to the bottom).
But this is just an hard guess.
We could maybe make buildContextMenu inject the menuitems at the bottom and stop using ordinal and see if that solves the problem, or ask someone from layout to debug this.
Blocks: 1248268
No longer blocks: 1248267
Marco, I think you're right about the cause in comment 15 and I also think that not using ordinal in this case would fix it.
Unfortunately I don't have the time _right now_ to tackle this issue. Dão, do you have time to take a look at this, perhaps?
Assignee: mdeboer → nobody
Status: ASSIGNED → NEW
Iteration: 52.2 - Oct 17 → ---
Flags: needinfo?(dao+bmo)
Attached patch patchSplinter Review
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(dao+bmo)
Attachment #8800606 - Flags: review?(mak77)
Comment on attachment 8800606 [details] [diff] [review]
patch

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

I assume you verified this fixes the problem, since my suggestion was untested :)
Attachment #8800606 - Flags: review?(mak77) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3e56f1eb513
Avoid using the ordinal attribute for positioning the Hide / Show Recently Bookmarked menu items. r=mak
https://hg.mozilla.org/mozilla-central/rev/b3e56f1eb513
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Hi Dao, should we uplift this fix to Beta50, Aurora51?
Flags: needinfo?(dao+bmo)
Flags: needinfo?(dao+bmo)
Summary: [Regression] Bookmark menu context menu item order is reversed after right click a folder → [Regression] Bookmark menu context menu item order is messed up after right-clicking a folder
Comment on attachment 8800606 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1248268
[User impact if declined]: see comment 0
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: very simple patch, low risk
[String/UUID change made/needed]: none
Attachment #8800606 - Flags: approval-mozilla-beta?
Attachment #8800606 - Flags: approval-mozilla-aurora?
Hello fireattack, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(human.peng)
Comment on attachment 8800606 [details] [diff] [review]
patch

Fixes a recent regression, low risk, Aurora51+, Beta50+
Attachment #8800606 - Flags: approval-mozilla-beta?
Attachment #8800606 - Flags: approval-mozilla-beta+
Attachment #8800606 - Flags: approval-mozilla-aurora?
Attachment #8800606 - Flags: approval-mozilla-aurora+
can confirm at least fixed in 52.
Status: RESOLVED → VERIFIED
Flags: needinfo?(human.peng)
(In reply to fireattack from comment #28)
> can confirm at least fixed in 52.

Great! Thank you for a prompt verification.
Verified fixed FX 50b10, 51.0a2 (2016-10-27) Win 7.
You need to log in before you can comment on or make changes to this bug.