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

VERIFIED FIXED in Firefox 50

Status

()

Firefox
Bookmarks & History
P2
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: fireattack, Assigned: dao)

Tracking

({regression})

Trunk
Firefox 52
regression
Points:
5
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox49 wontfix, firefox-esr45 unaffected, firefox50+ verified, firefox51 verified, firefox52 verified)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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).
(Reporter)

Updated

2 years ago
Component: Untriaged → Bookmarks & History
(Reporter)

Updated

2 years ago
Blocks: 1248268
(Reporter)

Updated

2 years ago
Keywords: regression

Updated

2 years ago
status-firefox49: --- → wontfix
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox-esr45: --- → unaffected
Flags: needinfo?(dao+bmo)
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?

Updated

2 years ago
Priority: -- → P2
(Reporter)

Comment 3

2 years ago
>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.
Duplicate of this bug: 1305272
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

Comment 7

2 years ago
hide-this
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
(Reporter)

Comment 8

2 years ago
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.
(Reporter)

Comment 10

2 years ago
(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)

Comment 11

2 years ago
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.
tracking-firefox50: --- → +
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.
(Reporter)

Updated

2 years ago
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)
(Assignee)

Comment 17

2 years ago
Created attachment 8800606 [details] [diff] [review]
patch
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+

Comment 19

2 years ago
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

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b3e56f1eb513
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Hi Dao, should we uplift this fix to Beta50, Aurora51?
Flags: needinfo?(dao+bmo)
(Assignee)

Updated

2 years ago
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
(Assignee)

Comment 22

2 years ago
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+

Comment 25

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/0daf6c792710
status-firefox51: affected → fixed

Comment 27

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/2f3602ef7f01
status-firefox50: affected → fixed
(Reporter)

Comment 28

2 years ago
can confirm at least fixed in 52.
Status: RESOLVED → VERIFIED
status-firefox50: fixed → affected
status-firefox52: fixed → verified
Flags: needinfo?(human.peng)
(Reporter)

Updated

2 years ago
status-firefox50: affected → fixed
(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.
status-firefox50: fixed → verified
status-firefox51: fixed → verified
You need to log in before you can comment on or make changes to this bug.