Closed Bug 1304617 Opened 4 years ago Closed 4 years ago
[Regression] Bookmark menu context menu item order is messed up after right-clicking a folder
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).
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?
>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
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?
I didn't rerun Mozregression, as the other bug has been duped, I trust the STRs and reg range found here.
(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!
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.
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 → ---
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
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 firstname.lastname@example.org: 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
Hi Dao, should we uplift this fix to Beta50, Aurora51?
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
Hello fireattack, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Comment on attachment 8800606 [details] [diff] [review] patch Fixes a recent regression, low risk, Aurora51+, Beta50+
can confirm at least fixed in 52.
(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.