Make focus/selection styling of (attachment) list items consistent with Windows Explorer on Windows default theme

ASSIGNED
Assigned to

Status

Thunderbird
Theme
ASSIGNED
23 days ago
4 days ago

People

(Reporter: Thomas D., Assigned: Paenglab, NeedInfo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 6 obsolete attachments)

45.64 KB, image/png
Details
57.93 KB, image/png
Details
11.95 KB, patch
Thomas D.
: review?
aceman
Details | Diff | Splinter Review
15.39 KB, patch
Paenglab
: feedback?
Thomas D.
Details | Diff | Splinter Review
(Reporter)

Description

23 days ago
Created attachment 8923283 [details]
Screenshot 1: Windows 10 Explorer focus ring with multiple selected items

+++ This bug was initially created as a clone of Bug #1410745 +++

(In reply to Thomas D. (currently busy elsewhere) from bug 1410745 comment #7)
> I just realize that it's better than before, but it's still not
> correct/perfect.
> 
> Please try to make it exactly as in windows Explorer, which is almost
> exactly as in contacts sidebar:
>
> - selected, but not focused:
>   - no border on left and right,
>   - inside border with weaker border-color,
>   - no outside border at selection edge (no border-top of top item /
>      no border-bottom of bottom item of selection).
>
> - selected and focused:
>   - 4-sides border (it's the only 4-sides border which makes it stand out)
>   - emphasized with stronger border-color
>   - (no background-color change)
> 
> Contacts sidebar is almost there, but two minor flaws:
> - selected & focused item has stronger top border, which isn't intended I
> think

The stronger border is unintended because it just combines the weak bottom border of the unfocused item with the strong border of the focused item.
So there must be something like a rule for the previous sibling of [focused & selected] item that it mustn't have a bottom border.

> - last selected item has bottom border and shouldn't
> 
> After that, we can test/discuss if there's any way to emphasize the 4-side
> focus border more, I guess just making it a bit darker would suffice (thicker
> might look odd and cause jumping problems).

Btw, Windows Explorer does NOT change the background color of focused item, because it's still an equal part of the selection, and the only thing which really matters is the focus ring, which used to be dotted, but is now plain.

We could also try the same if we make the focus ring a bit more emphasized (darker).
(Reporter)

Comment 1

23 days ago
Also, this selection pattern should be the same for all types of multiple-selection listboxes that we have, so we should probably use color variables instead of hard-coded absolute colors.
Or maybe those variables can even hold the entire styling? That would make things a lot easier than repeating the same styling all over.
(Reporter)

Comment 2

23 days ago
Comment on attachment 8923283 [details]
Screenshot 1: Windows 10 Explorer focus ring with multiple selected items

Screenshot is Windows 10 default theme without any customizations that I'm aware of.
Attachment #8923283 - Attachment description: Screenshot 1: Windows Explorer focus ring with multiple selected items → Screenshot 1: Windows 10 Explorer focus ring with multiple selected items
(Assignee)

Comment 3

19 days ago
Created attachment 8925130 [details] [diff] [review]
1412758.patch

This patch copies the rules of the toolkit tree.css to the attachmentItems. This is all we can do with CSS. CSS can't style a element depending it's subsequent elements state. This is also why the treeitems aren't 100% like Explorer.

Jörg, you can check Win7 and Thomas you can check Win10.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8925130 - Flags: review?(jorgk)
Attachment #8925130 - Flags: feedback?(bugzilla2007)
(Reporter)

Comment 4

18 days ago
Created attachment 8925253 [details] [diff] [review]
Partial Patch (on top of my 2 other reorder attachments patches): for attachmentBucketOnSelect event, mark first and last of selected blocks to allow CSS styling

Note: This patch requires my 2 other patches:
- reorder attachments
- reorder attachments via dnd

(In reply to Richard Marti (:Paenglab) from comment #3)
> Created attachment 8925130 [details] [diff] [review]
> 1412758.patch
> 
> This patch copies the rules of the toolkit tree.css to the attachmentItems.
> This is all we can do with CSS. CSS can't style a element depending it's
> subsequent elements state. This is also why the treeitems aren't 100% like
> Explorer.

Indeed there's no CSS-only solution, which looks like a feature gap in CSS, and I'm not the first one to notice that gap. There's last-of-type but that's only for child elements, not for other selectors. But where there's a will there's a way... Let's go for 100%!

Please try something along the lines of my partial patch which uses attachmentBucketOnSelect() function to mark items like this:

<attachmentitem selected="true" selectedFirstOfBlock="true"> (first of selected block)
<attachmentitem selected="true"> (middle of selected block)
<attachmentitem selected="true" selectedLastOfBlock="true"> (last of selected block)

With that, I think it will be possible and quite easy to apply the correct style. In fact, probably my two other patches could be even more simplified and perform faster if we just check for these attributes instead of determining block corners again in every routine. So we just do one loop over selected items to determine adjacent blocks and mark them with attributes, then we're done and all other algorithms can take it from there. Nice!

> Jörg, you can check Win7 and Thomas you can check Win10.

I would prefer to check when my suggested improvements have been tried and applied.
(Assignee)

Comment 5

18 days ago
Thomas, have you already checked how my patch works?

I would prefer you try my patch before you propose additional changes.
(In reply to Richard Marti (:Paenglab) from comment #3)
> This patch copies the rules of the toolkit tree.css to the attachmentItems.
> This is all we can do with CSS. CSS can't style a element depending it's
> subsequent elements state. This is also why the treeitems aren't 100% like
> Explorer.
Can you please describe what the patch is meant to do. I can see that it affects multiple selections in the attachment bicked in the compose window.

I see this:
Windows 7 explorer (as comparison):
When multiple items are selected, they all have the same colour. I hovered selected item is a little darker. On Win7 there is no focus ring.

Daily without the patch:
When multiple items are selected, they all have the same colour. Or maybe the last item selected (focus?) is a little darker, hart to tell. Hovering the selection does nothing.

With the patch:
When multiple items are selected, they all have the same colour. Or maybe the last item selected (focus?) is a little darker, hart to tell. Hovering the last selected item make it lighter, hovering the other selected items makes them darker. Is that intended? That's not like Explorer.

Questions:
Do all the selected items have the same colour, or is there one different? Like the one that's hovered differently?
When you say "tree items", which tree do you mean? The folder tree? There (and in the contacts sidebar) the last selected item in a multiple selection doesn't show a hover effect, the others do.
(Reporter)

Comment 7

15 days ago
(In reply to Richard Marti (:Paenglab) from comment #3)
> Created attachment 8925130 [details] [diff] [review]
> 1412758.patch
> 
> Jörg, you can check Win7 and Thomas you can check Win10.

Alright, thank you very much for the new patch (and I'm really glad that you're doing this...)
The new styles are a significant improvement over the status quo, and now mimicking Windows Explorer pretty well.

That said, maybe we could still polish this a bit more?

1) As I said before, please do remove the focus ring on the attachment list, it's inconsistent with OS and our own standards, and it really just confuses without adding value. Anything blue in my window with a blue border is a clear indication of focus, no need to duplicate focus ring which is actually wrong.

2) I'm not sure, but attachment items look a bit squeezed now, compared to contacts side bar where the spacing between items seems to be 1 or 2 px more generous, which looks better imo.

3) I might be wrong, but I believe that Windows 10 Explorer *does* have a slightly different background shading for the focused item among selected items. But it's practically invisible. Maybe not needed.

4) I'm still wondering how we can emphasize the focused item between selected items a bit more. I'm having a hard time even in Explorer to find the focused item. Good old dotted had the advantage of actually being visible, but maybe it looks too old now. I guess the choices are:
- darker focus border
- thicker focus border (but it's less elegant)
(- slightly different background color)
(- different style focus border)
(- different color focus border)

So far for the CSS-only nits. These are really cosmetic, meaning you did a great job there.
Beyond that, I suggest that we add just a tiny little bit of code to set 3 attributes on items which will allow as to fix the remaining nits in CSS also. Moreover, I believe that having these attributes could significantly simplify and improve performance of all of my re-ordering algorithms, so we'd probably implement this anyway.

3) Please use my code snippet of attachment 8925253 [details] [diff] [review] to get the following attributes on items:
> selectedFirstOfBlock="true" OR
> selectedLastOfBlock="true"
I'll add 2 lines of code so that we also have:
> selectedBeforeFocusedItem="true"

4) As I said before, let's please get rid of the double emphasis of focus-border-top. That's an artefact because with CSS-only, you couldn't remove the border-bottom of selected-item-before-focused-item. Now you can, by checking for [selectedBeforeFocusedItem="true"]

5) As I said before, let's be perfect and remove a useless bottom border on the last selected item if it doesn't have focus. Now you can, check for 'not focused' and [selectedLastOfBlock="true"] (or maybe focused is specific enough to overwrite anything else, or you can have !important on the focused item rules to enforce them, but be careful wrt 6) below).

6) For my bug 229224, please create CSS rules which allows me to show a dropmarker border-top based on [dropOn="top"] on
- selected but not focused items
- selected AND focused items
(see bug 229224 comment 37, and bug 229224 comment 34 ff. for details why this is needed).
(Reporter)

Comment 8

15 days ago
7) I suggest that we find a better alternative for what is very confusing in Windows 10 Explorer:
- hovering *non-selected* items does NOT create a focus-border effect (that's good), just shading background a bit darker in a light color, lighter than selection (that's good).
- hovering a *selected* item with mouse will *not* change background-color (or change is practically invisible), but only add a border-effect which is same as focus ring, so visually there are now 2 focus rings. It is very easy to have two focus rings even when your mouse is just "parked" in a position which is still inside selected items. Which is very confusing, and imo conceptually wrong.

(Also note the gross error in Windows Explorer that hovering a single non-selected item between selected items will cause a different (darker!) background color than on free-floating non-selected items. Confusing as now you don't see that this item isn't even selected when you hover it. I'm happy to see that TB gets that right!)

When hovering selected items, what if we apply the same type of change as for unselected items, i.e. just change the background-color of selected and hovered items to something slightly *darker* than selected, but *not* add a focus ring. As an avid keyboard user, I'm a strong fan of single focus...
(Reporter)

Comment 9

14 days ago
(In reply to Thomas D. from comment #7)
> 3) Please use my code snippet of attachment 8925253 [details] [diff] [review]
> [review] to get the following attributes on items:
> > selectedFirstOfBlock="true" OR
> > selectedLastOfBlock="true"
> I'll add 2 lines of code so that we also have:
> > selectedBeforeFocusedItem="true"

I've added 5 lines of simple, straightforward, and light-performing code to realize this (selectedBeforeFocusedItem attribute). Unfortunately, there's an unexpected existing bug which makes this fail:
The newly focused item is not yet updated when onSelect event fires on the listbox. :/
That's quite wrong and absurd because obviously changing selection normally involves changing focus first (but not for some keyboard selection methods like Ctrl+A or Ctrl+Space), and whenever focus change effects selection change the focus change obviously comes first. Imo, this means that onselect is currently triggering after the real focus change but before focus change has been completed programmatically which looks wrong to me.
So far I don't have ideas how to work around that, anyone?

How can I find the internal code which triggers onSelect event of listbox?

Here's the full updated function:

> function markAttachmentsSelectedBlocksBoundaries() {
>   // Mark the first and last item of adjacent selected attachment items (blocks),
>   // and a selected item before a focused selected item,
>   // to allow special borderless CSS.
>   let bucket = document.getElementById("attachmentBucket");
>   let selItems = attachmentsSelectedItemsGetSortedArray();
>   selItems.forEach(item => {
>     console.log(item.attachment.name + ' current=' + item.getAttribute("current") + '; currentItem = ' + bucket.currentItem.attachment.name);
>     let previousItem = item.previousSibling;
>     let nextItem = item.nextSibling;
>     if (!previousItem || !previousItem.selected) {
>       item.setAttribute("selectedFirstOfBlock", "true");
>     } else {
>       // Removing a non-existing attribute isn't a problem, but faster.
>       item.removeAttribute("selectedFirstOfBlock");
>     }
>     if (!nextItem || !nextItem.selected) {
>       // No next item, or next item not selected, so current item is end of block.
>       item.setAttribute("selectedLastOfBlock", "true");
>       item.removeAttribute("selectedBeforeFocusedItem");
>     } else if (nextItem.getAttribute("current") == "true") {
>       // nextItem is selected and focused, mark current item for special CSS.
>       item.setAttribute("selectedBeforeFocusedItem", "true");
>       item.removeAttribute("selectedLastOfBlock");
>     } else { // nextItem is selected, but not focused.
>       // Removing a non-existing attribute isn't a problem, but faster.
>       item.removeAttribute("selectedLastOfBlock");
>       item.removeAttribute("selectedBeforeFocusedItem");
>     }
>   })
> }
Flags: needinfo?(jorgk)
Flags: needinfo?(acelists)

Comment 10

14 days ago
Try around https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/toolkit/content/widgets/listbox.xml#692 .
Flags: needinfo?(acelists)
I'm afraid I can't contribute an answer here. Deep in my heart, I'm still a C++ programmer. That skill in fact fixes the most immediate bustages (which is my job), and I leave the tricky JS/XML/etc. stuff to others ;-)
Flags: needinfo?(jorgk)
Comment on attachment 8925130 [details] [diff] [review]
1412758.patch

Clearing the review for now. My comments are in comment #6. I asked some questions there, which haven't been answered, unless I missed something. I can repeat that I don't think that the hover behaviour is quite right, hovering "lighter" and "darker" seems confusing.

Also, it appears that we won't go ahead with this patch in it's current form, so it doesn't have to flash red in my face.
Attachment #8925130 - Flags: review?(jorgk)
(Reporter)

Comment 13

14 days ago
And also strangely, onSelect() event does not fire when items are de-selected, so this doesn't work very well (which also makes me doubt if updating commands in onSelect() does anything useful because it will fail to trigger in 50% of cases; maybe that's why we update them again before showing any menus...).

Any ideas how to reliably catch the events when selected items and/or focus change in the listbox?

Or we need to create our own mutation observer?
(Reporter)

Comment 14

14 days ago
Comment on attachment 8925253 [details] [diff] [review]
Partial Patch (on top of my 2 other reorder attachments patches): for attachmentBucketOnSelect event, mark first and last of selected blocks to allow CSS styling

Sorry, this routine is faulty and doesn't remove the attributes correctly when they are outside the current selection.
I'll offer another iteration soon.
I think I also found the spot to fix the currentItem problem for onSelect event in listbox.xml.

(In reply to Thomas D. from comment #13)
> And also strangely, onSelect() event does not fire when items are
> de-selected, so this doesn't work very well (which also makes me doubt if
> updating commands in onSelect() does anything useful because it will fail to
> trigger in 50% of cases; maybe that's why we update them again before
> showing any menus...).

Scrap that comment. My routine didn't respond correctly because I was only looking at selectedItems, so I forgot to handle the case where Selection is removed.
Attachment #8925253 - Attachment is obsolete: true
(Reporter)

Comment 15

13 days ago
Created attachment 8926744 [details] [diff] [review]
Partial Patch V.2 (on top of attachment 8924354 [details] [diff] [review] & attachment 8923082 [details] [diff] [review]): for attachmentBucket onSelect event, add selection marker attributes to allow precise CSS styling

Richard, this mostly works now to create the attributes needed for more precise CSS styling (except for some edge cases) (and simplification + performance improvements for reordering algorithms). So maybe you could try it now (cf. comment 7 bottom half). Just checking for the presence of the following attributes on attachmentitems will be enough, because they are either true or non-existent:

selectedFirstOfBlock --> hmm, do we need this? Maybe not.
selectedLastOfBlock  --> no bottom border
selectedBeforeFocusedItem --> no bottom border

Known issues:
1) Moving focus ring across existing selection will not update selectedBeforeFocusedItem (cosmetic glitch); we'll need to track every focus change in attachmentBucket for that. Maybe mutation observer for current="true" attribute on attachmentitems is the easiest, rather than messing in every corner of listbox.xml.

2) There's an existing edge case bug that with existing selection, using Ctrl+Shift+Space on a non-adjacent, non-selected item mostly does *not* do the right thing. Worse, it sometimes does something when first item was mouse-selected, and something else when it was keyboard-selected. Existing selection algorithms of the listbox are really messy. Accordingly, I failed to make Ctrl+Shift+Space do the right thing in listbox.xml, because the internals already spoil the selection. We need to know the last selected item (in time, not index) to get it right, pls try Windows Explorer behaviour: Select item 1, item 5, item 3 (in that order), then Ctrl+Cursor to move focus to item 7, press Ctrl+Shift+Space. Expected result: items 1 and items 3-7 are selected.

Aceman, could you point me to the deeper internals where listbox selection is defined? listbox.xml just seems to be just another secondary layer. Or maybe I don't understand this code, which is also possible.

Could you also let me know if my changes in listbox.xml make sense, I was trying to set the new current=focused item *before* firing onSelect event on the listbox, because focus change obviously occurs before selection, and we need to know the correct focus in the onSelect event. I'm not sure if doing this might have any other side effects; I tweaked selectItemRange() to avoid some.

What happens if there are 100 selected attachments and selection changes before my selection-marking algorithm is finished? Unfortunately, I don't know anything about performance testing.
Flags: needinfo?(richard.marti)
Flags: needinfo?(acelists)
(Reporter)

Comment 16

13 days ago
For a simple selection failure test, try this in current TB release:

Mouse-select item 1.
Hold Ctrl+Shift while mouse-selecting item 3.
Actual: 1+3 selected (wrong)
Expected: 1-3 selected.

Now doing the same, keyboard only.
Keyboard-select item 1 (do NOT use your mouse in any way, use cursor keys, ctrl+Space and such).
Ctrl+Cursor down to move focus on item 3 without losing your selection of item 1.
Ctrl+Shift+Space on item 3.
Actual: 3 selected (double wrong, unexpected AND different from mouse method!)
Expected 1-3 selected.

Try the same in Windows Explorer to see correct, expected behaviour.
(Reporter)

Comment 17

13 days ago
BTW, I'm seeing massive delays (TB not responding) of 60 seconds and more, both on flat install and normal release, when opening a simple draft message from POP account with 14 plain text attachments (different extensions) totalling 1KB (sic!). I wouldn't claim my laptop is very fast, but I have 12 GB or RAM and I'm surprised to see daily with 30% CPU load while trying to load that simple message. Jörg, any comments/ideas? Has anyone seen such?
Flags: needinfo?(jorgk)
We're off topic here, right? Compact and/or repair the drafts folder. Maybe related to bug 1415723.
Flags: needinfo?(jorgk)
(Reporter)

Comment 19

13 days ago
(In reply to Thomas D. from comment #16)
> For a simple selection failure test, try this in current TB release:
> 
> Mouse-select item 1.
> Hold Ctrl+Shift while mouse-selecting item 3.
> Actual: 1+3 selected (wrong)
> Expected: 1-3 selected.
> 
> Now doing the same, keyboard only.
> Keyboard-select item 1 (do NOT use your mouse in any way, use cursor keys,
> ctrl+Space and such).
> Ctrl+Cursor down to move focus on item 3 without losing your selection of
> item 1.
> Ctrl+Shift+Space on item 3.
> Actual: 3 selected (double wrong, unexpected AND different from mouse
> method!)
> Expected 1-3 selected.
> 
> Try the same in Windows Explorer to see correct, expected behaviour.

For completeness:

Only Mouse and keyboard mix does the right thing in TB release:
Mouse-select item 1. (important)
Ctrl+Cursor down to move focus on item 3 without losing your selection of item 1.
Ctrl+Shift+Space on item 3.
Actual = Expected: 1-3 selected.
(Assignee)

Comment 20

13 days ago
I never press CTRL+SHIFT. I use only CTRL to select only the actual item and SHIFT to select all items between the selected and the actual. It seems there is only one modifier defined and CTRL has precedence to SHIFT. Can't try now, I'm at work, what happens when you release all keys after moving to the item and press first SHIFT and then CTRL+Space?

But this selection failure is something for a new bug, maybe toolkit.
(Reporter)

Comment 21

13 days ago
(In reply to Richard Marti (:Paenglab) from comment #20)
> I never press CTRL+SHIFT. I use only CTRL to select only the actual item and
> SHIFT to select all items between the selected and the actual.

Ctrl+Shift and plain Shift do different things.
I can't say that the windows logic looks right to the detail, it seems to work best working downwards, not upwards or mixed. but this is what it seems to be (I might be wrong):

Windows behaviour:

Ctrl generally preserves the existing selection and adds to it.

Plain Shift will destroy at least disjunct selections.
On disjunct selections, it seems to select everything from the first item of the last selected block to the new item clicked with Shift (losing the rest of the last block - that feels wrong - and any initial selection blocks in the process). That behaviour on disjunct selections going upwards is not very intuitive, maybe just an artefact which wasn't tested. You need 3 disjunct blocks and then try adding to the middle one to see the effect.

With Ctrl+Shift, almost the same as plain Shift, but you don't lose the other items of the last block. Imo it's wrong that all gaps in between are selected, it should only select from the nearest block or maybe focused item to the Ctrl-shift-clicked item.

> It seems
> there is only one modifier defined and CTRL has precedence to SHIFT. Can't
> try now, I'm at work, what happens when you release all keys after moving to
> the item and press first SHIFT and then CTRL+Space?

I'm not seeing any change with that, and releasing all keys, or the order in which modifier keys are pressed, must not make a difference.

> But this selection failure is something for a new bug, maybe toolkit.

Yeah.
(Reporter)

Comment 22

11 days ago
(In reply to Thomas D. from comment #15)
> Created attachment 8926744 [details] [diff] [review]
> ...
> selectedBeforeFocusedItem --> no bottom border
> 
> Known issues:
> 1) Moving focus ring across existing selection will not update
> selectedBeforeFocusedItem (cosmetic glitch); we'll need to track every focus
> change in attachmentBucket for that. Maybe mutation observer for
> current="true" attribute on attachmentitems is the easiest, rather than
> messing in every corner of listbox.xml.

I've got the mutation observer for this case up and running correctly in my local install - surprisingly easy to set up. So Richard can just look at the selectedBeforeFocusedItem attribute in his CSS to remove the bottom border and hence avoid the apparent double top-border of the focused item within selected items.
(Reporter)

Updated

11 days ago
Summary: Make focus/selection pattern of (attachment) list items consistent with Windows Explorer on Windows default theme → Make focus/selection styling of (attachment) list items consistent with Windows Explorer on Windows default theme
(Reporter)

Comment 23

11 days ago
Created attachment 8927566 [details] [diff] [review]
Markers-Patch V.3: For attachmentBucket onSelect and 'currentItem-Change' events, add selection marker attributes for CSS styling

This patch now applies on current tip without other patches.
This patch should be applied (and imo also landed) separately before Richard's styling patch (I would like to keep my patch separate for correct attribution of patch author). Then Richard's patch will be able to use the following selection marker attributes for precise styling of focused/selected items via CSS:

(Caveat: I've changed the attribute name of selectedBeforeFocusedItem to selectedBeforeCurrentItem)

selectedFirstOfBlock --> hmm, do we need this? Seems currently not.
selectedLastOfBlock  --> no bottom border
selectedBeforeCurrentItem --> no bottom border

Richard, could you comment if this works for you?
Could you provide an updated patch on top of this patch?

We need to land this before bug 663695, assuming we want to use the block-marking algorithm of this patch to simplify and improve performance of bug 663695.
Attachment #8926744 - Attachment is obsolete: true
Attachment #8927566 - Flags: review?(acelists)
Attachment #8927566 - Flags: feedback?(richard.marti)
(Reporter)

Updated

11 days ago
Flags: needinfo?(richard.marti)
Flags: needinfo?(acelists)
(Reporter)

Comment 24

11 days ago
Comment on attachment 8925130 [details] [diff] [review]
1412758.patch

I've provided full feedback in my comment 7. f+ because this is a significant improvement over the status quo, so definitely going into the right direction.

Please apply my patch attachment 8927566 [details] [diff] [review] and then update your patch to use the attributes provided by my patch for more precise styling exactly as in Windows Explorer.
Attachment #8925130 - Flags: feedback?(bugzilla2007) → feedback+
(Reporter)

Comment 25

11 days ago
Aceman, please review my new markers-patch attachment 8927566 [details] [diff] [review] with special attention to the following:

This patch uses onSelect event of attachmentBucket and iterates all selected attachment items to create these attributes:
> selectedFirstOfBlock
> selectedLastOfBlock

I believe we could use this to simplify and performance-improve the code of bug 663695, attachment 8923082 [details] [diff] [review], because there are a number of spots where I'm checking for the boundaries of selected blocks, so the new attributes would come in handy. But I first need your informed expert opinion on the following:

(In reply to Thomas D. from comment #15)
> What happens if there are 100 selected attachments and selection changes
> before my selection-marking algorithm is finished?
> Unfortunately, I don't know anything about performance testing.

STR:
1) With 100 selected attachments followed by 50 unselected attachments
2) Hold down (Shift + cursor down) to rapidly select more attachments (101-150)

Questions - Actual result:
1) Does this still work reasonably in terms of performance?
2) Does this still work 100% reliable to set the attributes?

Expected result:
- reasonable performance because of simple and efficient algorithms
- hopefully the algorithm does not get confused when it's called again before the first round is finished.

It would be great if you could comment on this so that I can update bug 663695 if we agree.
Btw, how far with writing up tests for bug 663695?

-----
OT: Toolkit listbox selection vs. currentItem issues

(In reply to Thomas D. from comment #15)
> Could you also let me know if my changes in listbox.xml make sense, I was
> trying to set the new current=focused item *before* firing onSelect event on
> the listbox, because focus change obviously occurs before selection, and we
> need to know the correct focus in the onSelect event. I'm not sure if doing
> this might have any other side effects; I tweaked selectItemRange() to avoid
> some.

Sorry I never included listbox.xml with my last patch. This is no longer needed here for correct styling because I'm now using the focus mutation observer to track the current attribute. I guess we could still try to fix this for ourselves or for toolkit, for which the next question is still relevant:

(In reply to Thomas D. from comment #15)
> Aceman, could you point me to the deeper internals where listbox selection
> is defined? listbox.xml just seems to be just another secondary layer. Or
> maybe I don't understand this code, which is also possible.

I believe there must be a deeper implementation layer of listbox because in keypress event of listbox.xml, I tried to capture Ctrl+Shift+Space to fix that selection algorithm but the selection was already done when coming to that event.
Flags: needinfo?(acelists)

Updated

11 days ago
Depends on: 663695
(Reporter)

Comment 26

11 days ago
Created attachment 8927573 [details] [diff] [review]
Markers-Patch V.3.1: For attachmentBucket onSelect and 'currentItem-Change' events, add selection marker attributes for CSS styling

Updated patch because bug 663695 has just landed in midair-collision.

Aceman: see comment 23, comment 25.
Richard: see comment 23, and:

(In reply to Thomas D. from comment #24)
> Comment on attachment 8925130 [details] [diff] [review]
> 1412758.patch
> 
> I've provided full feedback in my comment 7. f+ because this is a
> significant improvement over the status quo, so definitely going into the
> right direction.
> 

Please apply this patch on top of fresh tip, then:
> update your patch to use the attributes provided by my patch for more
> precise styling exactly as in Windows Explorer.
Attachment #8927566 - Attachment is obsolete: true
Attachment #8927566 - Flags: review?(acelists)
Attachment #8927566 - Flags: feedback?(richard.marti)
Attachment #8927573 - Flags: review?(acelists)
Attachment #8927573 - Flags: feedback?(richard.marti)

Comment 27

11 days ago
I have landed bug 663695 meaning you guys can now work on a common base.

(In reply to Thomas D. from comment #25)
> Aceman, please review my new markers-patch attachment 8927566 [details] [diff] [review]
> [diff] [review] with special attention to the following:

I haven't read this bug yet so I need to look at what are you trying to solve here.
 
> (In reply to Thomas D. from comment #15)
> > What happens if there are 100 selected attachments and selection changes
> > before my selection-marking algorithm is finished?
> > Unfortunately, I don't know anything about performance testing.
> 
> STR:
> 1) With 100 selected attachments followed by 50 unselected attachments
> 2) Hold down (Shift + cursor down) to rapidly select more attachments
> (101-150)
> 
> Questions - Actual result:
> 1) Does this still work reasonably in terms of performance?
> 2) Does this still work 100% reliable to set the attributes?

Thanks, I will try that use case to see how slow it is.
 
> Expected result:
> - reasonable performance because of simple and efficient algorithms
> - hopefully the algorithm does not get confused when it's called again
> before the first round is finished.
> 
> It would be great if you could comment on this so that I can update bug
> 663695 if we agree.

I decided to land bug 663695 to prevent it from deteriorating further. If this bug makes improvements/speedup to the feature, you can now do them here.

> Btw, how far with writing up tests for bug 663695?
Tests are slowly progressing.
 
> (In reply to Thomas D. from comment #15)
> > Aceman, could you point me to the deeper internals where listbox selection
> > is defined? listbox.xml just seems to be just another secondary layer. Or
> > maybe I don't understand this code, which is also possible.
> 
> I believe there must be a deeper implementation layer of listbox because in
> keypress event of listbox.xml, I tried to capture Ctrl+Shift+Space to fix
> that selection algorithm but the selection was already done when coming to
> that event.

I'm also no expert in this. If what you are looking for is not in the file, it may be in the file that listbox bindings inherit from. See extends= attribute on the bindings. Also, there may be some behaviour of XUL elements that is implemented in C++ code. Maybe that is the part they are wanting to get rid of in m-c.
(Assignee)

Comment 28

10 days ago
Created attachment 8927645 [details] [diff] [review]
1412758_CSS.patch

This should fix all issues except when hovering selected items. Like the 'selectedBeforeCurrentItem' attribute we'd need a 'selectedBeforeHoveredItem'.
Attachment #8925130 - Attachment is obsolete: true
Attachment #8927645 - Flags: feedback?(bugzilla2007)
(Assignee)

Comment 29

10 days ago
Comment on attachment 8927573 [details] [diff] [review]
Markers-Patch V.3.1: For attachmentBucket onSelect and 'currentItem-Change' events, add selection marker attributes for CSS styling

This works as described. :)
How I wrote in comment 28, a 'selectedBeforeHoveredItem' would be needed to hide the double border when hovering one of multiple selected items.
Attachment #8927573 - Flags: feedback?(richard.marti) → feedback+
(Reporter)

Comment 30

7 days ago
(In reply to Richard Marti (:Paenglab) from comment #29)
> Comment on attachment 8927573 [details] [diff] [review]
> Markers-Patch V.3.1: For attachmentBucket onSelect and 'currentItem-Change'
> events, add selection marker attributes for CSS styling
> 
> This works as described. :)
> How I wrote in comment 28, a 'selectedBeforeHoveredItem' would be needed to
> hide the double border when hovering one of multiple selected items.

It's no problem to code that up for you, but it seems to imply that you're using 'border' as an indicator of the hovered item amongst a block of selected items. What about my proposal of comment 8 where I suggested using slightly darker background-color instead of 'border' when hovering selected items?

Windows Explorer current behaviour (imo inconsistent and confusing):
- hovering a non-selected item: background-color slightly shaded (i.e. 'darker' than before; that's good).
- hovering a selected item: no change of background-color; instead: border-effect which looks exactly like a second focus-ring on hovered item (that's bad).

Imo, having what looks like two focus rings is confusing and conceptually wrong, because it's in the nature of "keyboard focus ring" to be unique.
It's easy to get double focus ring even without actively hovering, when your mouse just happens to be 'parked' somewhere inside the selected items. Depending on scenario, there's no way of telling whether the fake focus ring of mouse hovering is also your current keyboard focus ring (the might fall together, or your keyboard focus ring might be somewhere else out of view). The hovering indication is also needlessly inconsistent: background-marker vs. ring-marker.

My proposal:

Let's be better than Windows Explorer, and eliminate "double focus ring".
- hovering a selected item: background-color slightly shaded (i.e. a bit *darker* than a plain selected item); NO focus ring on hovered item unless it coincides with the real keyboard focus ring.

So then we'd have consistency between hovering a unselected and selected items, because in both cases we're using the same style of hover indication, to apply a slightly darker shading of background-color.
We can't use the same background-color as on hovered unselected items, because users need to be able to tell the difference between hovering an unselected vs. hovering a selecte item, esp. with unselected items in between non-adjacent selected items.

Richard, (Jörg, Aceman) what do you think?
Flags: needinfo?(richard.marti)
(Assignee)

Comment 31

7 days ago
(In reply to Thomas D. from comment #30)
> Imo, having what looks like two focus rings is confusing and conceptually
> wrong, because it's in the nature of "keyboard focus ring" to be unique.
> It's easy to get double focus ring even without actively hovering, when your
> mouse just happens to be 'parked' somewhere inside the selected items.
> Depending on scenario, there's no way of telling whether the fake focus ring
> of mouse hovering is also your current keyboard focus ring (the might fall
> together, or your keyboard focus ring might be somewhere else out of view).
> The hovering indication is also needlessly inconsistent: background-marker
> vs. ring-marker.

There is no "double focus ring" in Explorer. In TB it looks like a partly one because the bottom border of the previous selected item has a bottom border and this is limited to CSS which can't remove this without the helper functions. But also with this limitation there is no double focus ring.

> My proposal:
> 
> Let's be better than Windows Explorer, and eliminate "double focus ring".
> - hovering a selected item: background-color slightly shaded (i.e. a bit
> *darker* than a plain selected item); NO focus ring on hovered item unless
> it coincides with the real keyboard focus ring.
> 
> So then we'd have consistency between hovering a unselected and selected
> items, because in both cases we're using the same style of hover indication,
> to apply a slightly darker shading of background-color.
> We can't use the same background-color as on hovered unselected items,
> because users need to be able to tell the difference between hovering an
> unselected vs. hovering a selecte item, esp. with unselected items in
> between non-adjacent selected items.
> 
> Richard, (Jörg, Aceman) what do you think?

On Win7 - 10 the hover on a selected item looks exactly the same as a selected and focused item. But there are differences between the Win versions. Win7 and 8 makes them a bit darker with always the same border. Win10 differentiates with the border. SO Jörg will say: "Hey, it's already like this". ;) Aceman has no Windows newer than XP (as I know) which is no more supported.

What you propose is more or less the state without this patches. The aim of this bug is to be consistent with Explorer, and now you want a different behavior.

So I'm against your proposal.
Flags: needinfo?(richard.marti)
(Reporter)

Comment 32

7 days ago
Created attachment 8928484 [details]
Screenshot 1: Win10-double-focus-ring-on-hover.png

(In reply to Richard Marti (:Paenglab) from comment #31)
> (In reply to Thomas D. from comment #30)
> > Imo, having what looks like two focus rings is confusing and conceptually
> > wrong, because it's in the nature of "keyboard focus ring" to be unique.
> > It's easy to get double focus ring even without actively hovering, when your
> > mouse just happens to be 'parked' somewhere inside the selected items.
> > Depending on scenario, there's no way of telling whether the fake focus ring
> > of mouse hovering is also your current keyboard focus ring (the might fall
> > together, or your keyboard focus ring might be somewhere else out of view).
> > The hovering indication is also needlessly inconsistent: background-marker
> > vs. ring-marker.
> 
> There is no "double focus ring" in Explorer.

I can't make sense of that answer. Technically, there isn't. Visually, there is, as seen in the attached screenshot. Now please tell me which of the two rings is the real focus ring. You can't. So in terms of UX, clearly there's the impression of a double focus ring, which is a design flaw because there's no way to tell them apart. Mouse pointer does NOT always help to tell them apart, depending on your selection and scroll patterns, because the two rings can fall together OR the real keyboard focus ring could be hidden in another part of the selection which isn't in view.

> In TB it looks like a partly
> one because the bottom border of the previous selected item has a bottom
> border and this is limited to CSS which can't remove this without the helper
> functions. But also with this limitation there is no double focus ring.
> 
> > My proposal:
> > 
> > Let's be better than Windows Explorer, and eliminate "double focus ring".
> > - hovering a selected item: background-color slightly shaded (i.e. a bit
> > *darker* than a plain selected item); NO focus ring on hovered item unless
> > it coincides with the real keyboard focus ring.
> > 
> > So then we'd have consistency between hovering a unselected and selected
> > items, because in both cases we're using the same style of hover indication,
> > to apply a slightly darker shading of background-color.
> > We can't use the same background-color as on hovered unselected items,
> > because users need to be able to tell the difference between hovering an
> > unselected vs. hovering a selecte item, esp. with unselected items in
> > between non-adjacent selected items.
> > 
> > Richard, (Jörg, Aceman) what do you think?
> 
> On Win7 - 10 the hover on a selected item looks exactly the same as a
> selected and focused item.

Exactly THAT is the problem, because there must only ever be ONE (selected AND) FOCUSED item, to avoid any impression of double focus. I'm failing to understand how you're denying the problem.

> But there are differences between the Win
> versions. Win7 and 8 makes them a bit darker with always the same border.
> Win10 differentiates with the border. SO Jörg will say: "Hey, it's already
> like this". ;) Aceman has no Windows newer than XP (as I know) which is no
> more supported.
> 
> What you propose is more or less the state without this patches.

I disagree. We're making everything consistent with Windows Explorer (e.g. the entire styling of selections) and only deviate in one small detail: for hovering selected items, change background color instead of showing border which looks like another focus ring.

> The aim of this bug is to be consistent with Explorer, and now you want a
> different behavior.

I'm adjusting a one minor detail to improve and correct the UX.

> So I'm against your proposal.

That's alright if you can tell me the difference between the focus ring and the hover ring in my screenshot...
(Reporter)

Comment 33

7 days ago
And apart from the "double focus ring" IMPRESSION, there's still the issue of consistency:
On a non-selected item, hovering does NOT create the impression of a focused item with focus ring.
On a selected item, hovering DOES create the impression of (another) focused item with focus ring.
I see no reason for this inconsistency. So we can't even say that it acts like a preview of what you're going to get when you click, because there's no full preview effect when hovering non-selected items.

Fwiw, I have not yet tested your new patch, but I take it from your comments that you're mimicking the Explorer hover behaviour.
(Assignee)

Comment 34

7 days ago
(In reply to Thomas D. from comment #32)
> Created attachment 8928484 [details]
> Screenshot 1: Win10-double-focus-ring-on-hover.png
> 
> (In reply to Richard Marti (:Paenglab) from comment #31)
> > There is no "double focus ring" in Explorer.
> 
> I can't make sense of that answer. Technically, there isn't. Visually, there
> is, as seen in the attached screenshot. Now please tell me which of the two
> rings is the real focus ring. You can't. So in terms of UX, clearly there's
> the impression of a double focus ring, which is a design flaw because
> there's no way to tell them apart. Mouse pointer does NOT always help to
> tell them apart, depending on your selection and scroll patterns, because
> the two rings can fall together OR the real keyboard focus ring could be
> hidden in another part of the selection which isn't in view.

Oh I can ;) What you call double focus ring is *two* focus rings. A double focus ring is a focus ring in a focus ring.

And you need to separate keyboard navigation with mouse navigation. The focus is important for keyboard navigation to show where the "keyboard cursor" is to show where something changes when you chage something. With the mouse, the hover state gives the feedback where the mouse is to change something. For a mouse user, the focus of one selected item in a group of selected items isn't important. He wants the feedback where the mouse is (the hover is the soft focus of the mouse user). With this in mind it makes sense that focus and hover are looking the same. The focus stays until the mouse is clicked on a new item.

> Exactly THAT is the problem, because there must only ever be ONE (selected
> AND) FOCUSED item, to avoid any impression of double focus. I'm failing to
> understand how you're denying the problem.

Again, don't mix keyboard- and mouse usage. They are different.

> I'm adjusting a one minor detail to improve and correct the UX.
> 
> > So I'm against your proposal.
> 
> That's alright if you can tell me the difference between the focus ring and
> the hover ring in my screenshot...

Your screenshot is invalid as an example as he doesn't show the mouse cursor which is important for mouse usage.
(Reporter)

Comment 35

7 days ago
(In reply to Richard Marti (:Paenglab) from comment #34)
> (In reply to Thomas D. from comment #32)
> > Created attachment 8928484 [details]
> > Screenshot 1: Win10-double-focus-ring-on-hover.png

> Oh I can ;) What you call double focus ring is *two* focus rings. A double
> focus ring is a focus ring in a focus ring.

Two, double, duplicate - whichever you like best...
 
> And you need to separate keyboard navigation with mouse navigation.

That is exactly what I'm trying to do; both are methods in their own right, so their indicators should be distinct, but also we need to acknowledge that they are not mutually exclusive but users are free to use a sequential combination of both, picking their personal use pattern of least resistance.
 
> The focus is important for keyboard navigation to show where the "keyboard
> cursor" is to show where something changes when you chage something.

Yes.

> With the mouse, the hover state gives the feedback where the mouse is to change
> something.

Yes.

> For a mouse user, the focus of one selected item in a group of
> selected items isn't important.

Hmm, we don't know. I'm an avid keyboard user but nothing is stopping me from using mouse to click the first item of my selection and then continue expanding my selection with keyboard. In fact, I think I do that quite often.

> He wants the feedback where the mouse is

Yes, my proposal provides that, I'm just suggesting a different, more distinct, and more visible(!) type of feedback than we have now, namely change of background color of a hovered selected item instead of border ring.

> (the hover is the soft focus of the mouse user).

Yeah, right, *soft* (i.e. not yet fully applied) *mouse* focus (which is clearly different from *hard* *keyboard* focus...)

> With this in mind it makes
> sense that focus and hover are looking the same.

Sorry, but imho that clearly does NOT make sense, because as you say, mouse hover focal point is a...
> ...*soft* (i.e. not yet fully applied) *mouse* focus (which is clearly different from *hard* *keyboard* focus...)

> The focus stays until the
> mouse is clicked on a new item.

Yes, that's the problem. The keyboard focus ring stays (which is correct and required), and confusingly, there's now another mouse focus ring which looks no different from the keyboard focus ring, but they are fundamentally different because one is already applied (hard keyboard focus) and the other one isn't applied yet until you click (soft mouse focal point).
So if I want to continue with keyboard after having clicked with mouse (and there's nothing stopping me from doing that, it's actually an efficient least-resistance use pattern), I'm having a hard time figuring out which is which, and depending on scenario, I won't be able to tell at first sight without further clumsy actions even if I try.

> > Exactly THAT is the problem, because there must only ever be ONE (selected
> > AND) FOCUSED item, to avoid any impression of double focus. I'm failing to
> > understand how you're denying the problem.
> 
> Again, don't mix keyboard- and mouse usage. They are different.

Yes, that's why they should have different indications.

> > I'm adjusting a one minor detail to improve and correct the UX.
> > 
> > > So I'm against your proposal.
> > 
> > That's alright if you can tell me the difference between the focus ring and
> > the hover ring in my screenshot...
> 
> Your screenshot is invalid as an example as he doesn't show the mouse cursor
> which is important for mouse usage.

Sorry, windows PRINT key eliminates the mouse pointer. That doesn't make the screenshot invalid, maybe marginally incomplete. Yes, sometimes mouse pointer will allow you to tell, with some effort, which border ring belongs to mouse, and which one belongs to keyboard. But (on the danger of starting to sound like a broken record), sometimes you won't be able to tell, because if you believe to see only the mouse focus, as it's no different from the keyboard focus, you can't tell whether:
- mouse and keyboard focus fall together on one item OR
- the keyboard focus might still be hidden somewhere scrolled down

With my proposal, user can always tell hard keyboard focus and soft mouse focal point apart, without any effort, even if only one of them is visible, and even if they fall together. Plus we'd be removing the inconsistency that soft mouse focus is *sometimes* shown as change of background color, and *sometimes* shown as change of border. Also note in my screenshot how hard it is to actually tell to which item the borders belong, you have to check very closely for the tiny right and left borders, and for the mouse pointer. Change of background color might turn out to be a clearer distinction when basically it's the mouse pointer anyway which you have to look at to see which is which between keyboard and mouse marker.

Anyway, I give up for now, if we're not understanding each other, let's just mimick Explorer for now and get done with this because for the major part we agree. I may or may not file this issue again as a separate bug.
(Reporter)

Comment 36

6 days ago
Created attachment 8928714 [details] [diff] [review]
Markers-Patch V.4: For attachmentBucket onMouseOver, onSelect and 'currentItem-Change' events, add selection marker attributes for CSS styling

Here's my patch with the "selectedBeforeHoveredItem" attribute algorithm as requested by Richard.
Attachment #8927573 - Attachment is obsolete: true
Attachment #8927573 - Flags: review?(acelists)
(Reporter)

Updated

6 days ago
Attachment #8928714 - Flags: review?(acelists)
(Reporter)

Comment 37

6 days ago
Comment on attachment 8927645 [details] [diff] [review]
1412758_CSS.patch

Btw, thank you very much for the patch and for working on this, looking at the sophistication of the CSS markup, this is quite impressive!

Apart from the problem discussed above, that we copy the imo wrong style of Windows explorer where hovering a selected item generates what looks like a second focus ring:

This looks much better, nice actually.
I see that the border color 'highlight' (used on my dropmarker) is a more intense blue than what we're actually using to highlight the border of current item.
-> I suggest that we use highlight border color for current item.

I assume that dropmarker/item-border is 1px thick, so when I talk about 1px, I mean that thickness (maybe it's 2px?).

Then we must find a way that the dropmarker will still be visible if it's at the top of the first list item when that's focused, or at the bottom of last list item when that's focused, because if we just use border-highlight on dropmarker of focused item, it will blend in with the existing border-highlight of current item.
-> I think we can use outline for that, probably
-> after adding another 1px padding at top and bottom of list so that the outline-dropmarker doesn't touch the listbox border.

(For a focused item in the middle, I can add the dropmarker at the top/bottom of an adjacent unselected item, so it will be visible because it combines with the existing border of current item to a thicker border.)

As said before, contact list items have some more px of vertical height per item (item-padding maybe?), which looks a bit more generous, current attachment list looks a bit cramped.
-> Can you try that?

Dropmarker is now thinner than before, that's quite elegant (almost fragile), and removes the shaking of items. I guess it's still visible enough, is it?

Cool, we're getting there! :)
Attachment #8927645 - Flags: feedback?(bugzilla2007) → feedback+
(Assignee)

Comment 38

5 days ago
Created attachment 8929613 [details] [diff] [review]
1412758_CSS.patch

Thank you Thomas. This patch adds now the "selectedBeforeHoveredItem".

This patch is now ready for review and needs Thomas's patch to correctly work. But unfortunately Jörg doesn't accept review request. So we have to wait until the contract issues are solved.
Attachment #8927645 - Attachment is obsolete: true
Attachment #8929613 - Flags: feedback?(bugzilla2007)

Comment 39

4 days ago
Comment on attachment 8928714 [details] [diff] [review]
Markers-Patch V.4: For attachmentBucket onMouseOver, onSelect and 'currentItem-Change' events, add selection marker attributes for CSS styling

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

Why is all this code needed to achieve some visual effect? Shouldn't the effect be implemented in XUL?
But if Paenglab thinks this is the right place and likes the appearance, I can review just the code as a last resort ;)
But where are the speed improvements that we talked about? Where do the existing 'slow' functions change?
Also please remove any observers you add: gAttachmentsFocusObserver.disconnect() in ComposeUnload().
(Assignee)

Comment 40

4 days ago
(In reply to :aceman from comment #39)
> Why is all this code needed to achieve some visual effect? Shouldn't the
> effect be implemented in XUL?

How should this be done in XUL?
You need to log in before you can comment on or make changes to this bug.