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)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 52
People
(Reporter: fireattack, Assigned: dao)
References
Details
(Keywords: regression)
Attachments
(1 file)
3.45 KB,
patch
|
mak
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•8 years ago
|
Component: Untriaged → Bookmarks & History
Reporter | ||
Updated•8 years ago
|
Keywords: regression
Updated•8 years ago
|
status-firefox49:
--- → wontfix
status-firefox50:
--- → affected
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox-esr45:
--- → unaffected
Flags: needinfo?(dao+bmo)
Comment 1•8 years ago
|
||
I couldn't reproduce at the beginning, until I right clicked one of the recent bookmarks
Comment 2•8 years ago
|
||
btw, to me doesn't look like they are all inverted, it's just Open All in Tabs that shifts to the bottom?
Updated•8 years ago
|
Priority: -- → P2
Reporter | ||
Comment 3•8 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.
Comment 5•8 years ago
|
||
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! :)
Comment 6•8 years ago
|
||
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+
Updated•8 years ago
|
Iteration: --- → 52.1 - Oct 3
Points: --- → 5
Updated•8 years ago
|
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•8 years ago
|
||
Thanks for clearing things up Mike. I don't why mozregression told me it's 1248268 on my end.
Comment 9•8 years ago
|
||
(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•8 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•8 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)
Comment 12•8 years ago
|
||
(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:
--- → +
Updated•8 years ago
|
Iteration: 52.1 - Oct 3 → 52.2 - Oct 17
Comment 14•8 years ago
|
||
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.
Comment 15•8 years ago
|
||
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•8 years ago
|
Comment 16•8 years ago
|
||
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•8 years ago
|
||
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(dao+bmo)
Attachment #8800606 -
Flags: review?(mak77)
Comment 18•8 years ago
|
||
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•8 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•8 years ago
|
||
bugherder |
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)
Assignee | ||
Updated•8 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•8 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•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/0daf6c792710
Comment 26•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/0daf6c792710
Comment 27•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/2f3602ef7f01
Reporter | ||
Comment 28•8 years ago
|
||
can confirm at least fixed in 52.
Status: RESOLVED → VERIFIED
Flags: needinfo?(human.peng)
Reporter | ||
Updated•8 years ago
|
(In reply to fireattack from comment #28) > can confirm at least fixed in 52. Great! Thank you for a prompt verification.
Comment 30•8 years ago
|
||
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.
Description
•