Closed Bug 1613284 Opened 4 years ago Closed 4 years ago

Attachment pane view state machinery via keyboard shortcut (Alt+M) regressed

Categories

(Thunderbird :: Message Compose Window, defect)

Product:
Thunderbird
Component:
Message Compose Window
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 76.0

People

(Reporter: thomas8, Assigned: mkmelin)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

bug1613284_attachmentpane_toggle.patch
16.15 KB, patch
aleca
: review+
Details | Diff | Splinter Review

From Bug #1613004, comment 0:

Some other behaviour of the carefully-crafted shortcut-operated attachment pane state machinery is also broken (repeated Alt+M), e.g. with a focused but not selected attachment, unexpectedly bringing up the dialog whilst simultaneously hiding the bucket.

I'll file this as a stub, we can do more details as we go along.

Alice, can you please find this regression for us. Already broken in TB 68.6.

STR:
Start TB, write a message, press Alt+M: Result Empty attachment area appears. Then press Alt+M again.

Expected:
Empty attachment area is closed again.

Actual: File browser is opened :-(

Flags: needinfo?(alice0775)

Grrrr, what went broken in June last year, and the breakage made it into TB 68 beta at the time :-(
The offender must be somewhere below this line:
https://hg.mozilla.org/releases/comm-beta/rev/06327efa8b67bd2c793b8aedca893cdf45ccaf89#l3.1

Magnus, can you please get this fixed.

Alice, thank you so much, what would we do without you?

Flags: needinfo?(mkmelin+mozilla)
Regressed by: 1547699

And maybe we should have a test for this. Pressing Alt+M twice on a new message and looking at the result isn't rocket science.

I have investigated this bug and found something interesting:
Pressing Alt+M with focus on empty attachmentBucket triggers both of the following events on <richlistbox is="attachment-list" id="attachmentBucket"> : onclick and onkeypress (in that order).

So much wrong here... :/

Flags: needinfo?(mkmelin+mozilla)

Remove the over-engineering here.
At the heart of the issue, we had the accesskey(!!) m of the attachmentBucketCount acting as double as a hotkey. In combination with controls on that label... But, this is just a label and I see no reason for it to have an accesskey which we use to go to editable places - which this is obviously not. Then how do one use an accesskey?With alt, so Alt+m. What was the hotkey? Alt-m => Wrong/double things getting triggered.

For the menuitem, there was a lot of code to do different things with focus, but I'm not convinced that was useful at all. (And working differently from the contacts pane, which do always get focus no matter how it was opened.) How that set a temporary attribute on an element to keep track of how we came there was just wrong.

Finally, the "checked" on the command should not be used at all anymore, it's part of the observes xul functionality that will be dropped at some point.

The autoCheck was just near by. Removed since it does nothing. The attribute is lower-case autocheck...

Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #9135755 - Flags: review?(alessandro)

(Forgot to lint and such.)

Attachment #9135755 - Attachment is obsolete: true
Attachment #9135755 - Flags: review?(alessandro)
Attachment #9135757 - Flags: review?(alessandro)
Comment on attachment 9135757 [details] [diff] [review]
bug1613284_attachmentpane_toggle.patch

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

Looks good and the newly implemented test pass.
r+
Attachment #9135757 - Flags: review?(alessandro) → review+
Comment on attachment 9135757 [details] [diff] [review]
bug1613284_attachmentpane_toggle.patch

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

Sorry I have to revert the review, but upon further testing, I noticed that these changes remove the ability to trigger the file chooser dialog if an attachment is already present in the list.
This breaks a11y of that area for keyboard users.
Attachment #9135757 - Flags: review+ → review-

We may have been too subtle with how to actually add files.
This adds button to make it clear.

Attachment #9135757 - Attachment is obsolete: true
Attachment #9136073 - Flags: review?(alessandro)
Comment on attachment 9136073 [details] [diff] [review]
bug1613284_attachmentpane_toggle.patch

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

This patch seems to have bitrotted with the latest push from bug 1613004
Attachment #9136073 - Flags: review?(alessandro)

Unbitrotted.

Attachment #9136073 - Attachment is obsolete: true
Attachment #9136091 - Flags: review?(alessandro)
Comment on attachment 9136091 [details] [diff] [review]
bug1613284_attachmentpane_toggle.patch

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

This is good, even tho the UI now it doesn't look very good.

Better land this patch anyway as its scope is related to fixing a glaring issue.
I'll open a dedicated bug to upload mock-ups and improve the UI and UX of the attachment pane.
Attachment #9136091 - Flags: review?(alessandro) → review+

Don't forget to fix this in TB 68.x where it's sadly broken as well :-( - See comment #3. Thanks for the test :-)

(In reply to Magnus Melin [:mkmelin] from comment #7)

Remove the over-engineering here.

At the heart of the issue, we had the accesskey(!!) m of the attachmentBucketCount acting as double as a hotkey. In combination with controls on that label... But, this is just a label and I see no reason for it to have an accesskey which we use to go to editable places - which this is obviously not. Then how do one use an accesskey? With alt, so Alt+m. What was the hotkey? Alt-m => Wrong/double things getting triggered.

Well, before you remove the alleged "over-engineering", how about you try it in a working version?
EDIT: https://archive.mozilla.org/pub/thunderbird/nightly/2018/11/2018-11-21-10-00-42-comm-central/

Sure, the attachment bucket is not editable as such, but getting there conveniently was a major part of the original solution. Once there, you can continue using the keyboard for things like using the context menu to do other keyboard based actions, or hitting enter to open the file selection dialogue. What you call "over-engineering" was a finely tuned efficiency tool that two Thunderbird peers (myself and Aceman) worked on.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/e83059b0ea0e
fix attachment pane toggling. r=aleca DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED

For 68: not exactly sure how to fix this properly there. "The minimal fix". Given we had no complaints about this, I'd tend to just let it ride the release trains.

Re comment 16: not sure what you're referring to, but with the patch things will still allow what you describe, just in less code and without the bug. With the added button how to add via keyboard will be discoverable, which (since we got no complaints) this bug shows the earlier situation was not.

Target Milestone: --- → Thunderbird 76.0

UI improvements in bug 1625263.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/a583705f483a
followup - only do focus changes when the shown/hidden status changed. rs=bustage-fix

(In reply to Jorg K (GMT+1) from comment #16)

...how about you try it in a working version?
EDIT: https://archive.mozilla.org/pub/thunderbird/nightly/2018/11/2018-11-21-10-00-42-comm-central/

Yes, the above version was still fully working as designed.
Fwiw, and ftr of history:

Bisecting on comm-central, 2018-03-12 - 2019-07-08:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=59c0f6e4825c4db83eb64c4dbd6ed11aee7be98e&tochange=2f0f868999d819e189bd67142aea132efb8a25c4

That's different from the regression range of this bug (comment 2) because I also checked when single click on attachment pane whitespace was still fully working (related bug 1613004 just fixed half of that); whitespace-attaching broke long before attach dialogs started popping up at the wrong time (this bug 1613284).

No longer blocks: 1625263
Regressions: 1625263

More easily found if placed in the compose component

Component: Mail Window Front End → Message Compose Window
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: