Closed Bug 1545854 Opened 7 months ago Closed 6 months ago

in the Customize palette, menulists (like the View or Folder Location) activate directly

Categories

(MailNews Core :: XUL Replacements, defect)

x86_64
Linux
defect
Not set

Tracking

(thunderbird68+ fixed, thunderbird69 fixed)

RESOLVED FIXED
Thunderbird 69.0
Tracking Status
thunderbird68 + fixed
thunderbird69 --- fixed

People

(Reporter: aceman, Assigned: mkmelin)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

Something happened to menulists in recent trunk.
There are a few new glitches appeared:

  1. the folder mode picker has no label.
  2. when in the Customize palette, trying to drag menulists (like the Mode picker or the Folder location picker) causes them to activate directly (open their popup) in the palette and throw errors as they are not supposed to show their menupopups there (e.g. 'ReferenceError: gFolderTreeView is not defined customizeToolbar.xul:1:1' for the mode picker).

(In reply to :aceman from comment #0)

Something happened to menulists in recent trunk.
There are a few new glitches appeared:

  1. the folder mode picker has no label.

This one is probably Bug 1545700 (fix on autoland).

For the other one, I don't know if it's related to this: https://phabricator.services.mozilla.com/D27801#817222 but it's something that would need to be fixed anyway.

I was going to say "Related to bug 1519502", but that's nothing new since bug 1545700 has that one as a cause. I believe we need to port some of bug 1519502 in bug 1545263.

It seems bug 1545700 has landed, but didn't help with this problem.

Looks like m-c has the same issue: bug 1546060.

(In reply to Richard Marti (:Paenglab) from comment #4)

Looks like m-c has the same issue: bug 1546060.

Bug 1528268 will fix it, if it's the same issue.

Bug 1528268 landed, and bug 1546024, where bug 1546060 was duped to, is on autoland. So let's see.

menulist label is back.

But the activation in customize palette still exists. Also the location widget on menubar contains all the folders multiple times, but that could be our job to adapt the folder picker to some new event ordering in m-c's menupopup.js .

I think the remaining work is bug 1542715.

Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → WORKSFORME

The customize problem also happens on other menulists, not just folder picker. E.g. the folder view menulist/popup.

Should be on all the folder-menupopup ones yes.

I meant this one: https://searchfox.org/comm-central/source/mail/base/content/mainMailToolbox.inc.xul#390
I don't think that one has anything to do with folder-menupopup.

Summary: weird problems with menulists → in the Customize palette, menulists (like the View or Folder Location) activate directly

(In reply to :aceman from comment #7)

Also the location widget on menubar contains all the folders multiple times, but that could be our job to adapt the folder picker to some new event ordering in m-c's menupopup.js .

Bug 1552666.

Yes, case 2. still happens after all the fixes landed right now.

Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---

Well, Magnus told us in bug 1542715 comment #1 that he wants to resolve it over there.

Status: REOPENED → RESOLVED
Closed: 7 months ago6 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1542715

I'm not sure how that bug fixes menulists that are not folder-menupopup.

Maybe indeed a different issue.

Status: RESOLVED → REOPENED
Keywords: regression
Resolution: DUPLICATE → ---
Status: REOPENED → NEW

I wonder what kept them from activating earlier. A regression window could help.

Flags: needinfo?(alice0775)

Detailed STR(step by step) and screenshot(good bad), please.

Flags: needinfo?(alice0775)

Hard to screenshot, but easy STR:

  1. right click Main Toolbar and choose Customize...
  2. click the dropdown part of the "Folder Location" or "Mail Views" widgets (not the labels, but the menu)
  3. notice the menu of that widget pops open showing folders

In step 3 the widget should not open that menu while in customization.

Flags: needinfo?(alice0775)

Thank you so much, Alice, this is very helpful indeed!

Richard, something you could take a look at?

Flags: needinfo?(richard.marti)

This is what I can do with CSS. If you want it through JS, I'm not the right person.

Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Flags: needinfo?(richard.marti)
Attachment #9066791 - Flags: review?(mkmelin+mozilla)

So what was the regressing commit?
I think this has to be done via JS as there is code run at onopopupshowing events of the menulists. That code (function) may not exist when inside the palette. See comment 0 and I also see some other errors about missing functions.
So we must prevent the menulist from activating its menupopup ever.
The folder picker e.g. actively detects if it is in the customize palette and removes all its menuitems in the popup.

Comment on attachment 9066791 [details] [diff] [review]
1545854-no-menupopup-when-customize.patch

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

This is fairly ok, but there's actually a css way to prevent actions. Will attach a patch in a sec.
Attachment #9066791 - Flags: review?(mkmelin+mozilla)

This works for me

Attachment #9066791 - Attachment is obsolete: true
Attachment #9067975 - Flags: review?(richard.marti)

For the regression range, agreed it must somehow be from bug 1461793.

Comment on attachment 9067975 [details] [diff] [review]
1545854-no-menupopup-when-customize.patch

Yes, better solution.
Attachment #9067975 - Flags: review?(richard.marti)
Attachment #9067975 - Flags: review+
Attachment #9067975 - Flags: approval-comm-beta?
Assignee: richard.marti → mkmelin+mozilla
Keywords: checkin-needed

If this supresses all events going to the menulists, then their code will not run, that looks great.
Can we check if other elements need this treatment too? Toolbar buttons seem OK, but maybe something else, like 'button type=menu' ?

(In reply to :aceman from comment #28)

If this supresses all events going to the menulists, then their code will not run, that looks great.
Can we check if other elements need this treatment too? Toolbar buttons seem OK, but maybe something else, like 'button type=menu' ?

Good catch. I tried it only in the customize window and they didn't popup...but now I checked it on the toolbar too and they do. :-( Fixed it with adding toolbarpaletteitem toolbarbutton.

Magnus, are you okay with my change?

Attachment #9068083 - Flags: review+
Attachment #9068083 - Flags: feedback?(mkmelin+mozilla)
Attachment #9068083 - Flags: approval-comm-beta?
Comment on attachment 9068083 [details] [diff] [review]
1545854-no-menupopup-when-customize.patch

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

LGTM
Attachment #9068083 - Flags: feedback?(mkmelin+mozilla) → feedback+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c999415db730
Don't show the menulist's menupopup when in customize mode. r=Paenglab

Status: ASSIGNED → RESOLVED
Closed: 6 months ago6 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 69.0
Attachment #9067975 - Attachment is obsolete: true
Attachment #9067975 - Flags: approval-comm-beta?
Attachment #9068083 - Flags: approval-comm-beta? → approval-comm-beta+

True, I also only now see that even toolbarbuttons, can be clicked in the palette.
So then why can't we disable all 'toolbarpaletteitem *', would dragging then not work?

(In reply to :aceman from comment #32)

True, I also only now see that even toolbarbuttons, can be clicked in the palette.
So then why can't we disable all 'toolbarpaletteitem *', would dragging then not work?

I'm not so a fan of adding a wildcard as childs of toolbarpaletteitem.

We want to disable everything in the paletteitem:)
'toolbarpaletteitem {...}' isn't good it disables even the dragging.
But 'toolbarpaletteitem > * {...}' seems to work for me.

You need to log in before you can comment on or make changes to this bug.