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

Status: RESOLVED FIXED

(Thunderbird :: Theme, enhancement)

Description

[Thomas D. (:thomas8)]

Attachment: 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).

Comment 1

[Thomas D. (:thomas8)]

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.

Comment 2

[Thomas D. (:thomas8)]

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.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: 1412758.V1.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.

Comment 4

[Thomas D. (:thomas8)]

Attachment: 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.

Comment 5

[Richard Marti (:Paenglab)]

Thomas, have you already checked how my patch works?

I would prefer you try my patch before you propose additional changes.

Comment 6

[Jorg K (CEST = GMT+2)]

(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.

Comment 7

[Thomas D. (:thomas8)]

(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).

Comment 8

[Thomas D. (:thomas8)]

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...

Comment 9

[Thomas D. (:thomas8)]

(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");
>     }
>   })
> }

Comment 10

[:aceman]

Try around https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/toolkit/content/widgets/listbox.xml#692 .

Comment 11

[Jorg K (CEST = GMT+2)]

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 ;-)

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8925130 [details] [diff] [review]
1412758.V1.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.

Comment 13

[Thomas D. (:thomas8)]

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?

Comment 14

[Thomas D. (:thomas8)]

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.

Comment 15

[Thomas D. (:thomas8)]

Attachment: Partial Patch V.2 (on top of attachment 8924354 & attachment 8923082): 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.

Comment 16

[Thomas D. (:thomas8)]

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.

Comment 17

[Thomas D. (:thomas8)]

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?

Comment 18

[Jorg K (CEST = GMT+2)]

We're off topic here, right? Compact and/or repair the drafts folder. Maybe related to bug 1415723.

Comment 19

[Thomas D. (:thomas8)]

(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.

Comment 20

[Richard Marti (:Paenglab)]

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.

Comment 21

[Thomas D. (:thomas8)]

(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.

Comment 22

[Thomas D. (:thomas8)]

(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.

Comment 23

[Thomas D. (:thomas8)]

Attachment: 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.

Comment 24

[Thomas D. (:thomas8)]

Comment on attachment 8925130 [details] [diff] [review]
1412758.V1.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.

Comment 25

[Thomas D. (:thomas8)]

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.

Comment 26

[Thomas D. (:thomas8)]

Attachment: 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.

Comment 27

[:aceman]

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.

Comment 28

[Richard Marti (:Paenglab)]

Attachment: 1412758_CSS.V2.patch

This should fix all issues except when hovering selected items. Like the 'selectedBeforeCurrentItem' attribute we'd need a 'selectedBeforeHoveredItem'.

Comment 29

[Richard Marti (:Paenglab)]

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.

Comment 30

[Thomas D. (:thomas8)]

(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?

Comment 31

[Richard Marti (:Paenglab)]

(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.

Comment 32

[Thomas D. (:thomas8)]

Attachment: 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...

Comment 33

[Thomas D. (:thomas8)]

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.

Comment 34

[Richard Marti (:Paenglab)]

(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.

Comment 35

[Thomas D. (:thomas8)]

(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.

Comment 36

[Thomas D. (:thomas8)]

Attachment: 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.

Comment 37

[Thomas D. (:thomas8)]

Comment on attachment 8927645 [details] [diff] [review]
1412758_CSS.V2.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! :)

Comment 38

[Richard Marti (:Paenglab)]

Attachment: 1412758_CSS.V3.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.

Comment 39

[:aceman]

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().

Comment 40

[Richard Marti (:Paenglab)]

(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?

Comment 41

[Thomas D. (:thomas8)]

Aceman, when you have time after 29 Dec, you are welcome to review my attribute-emitting code...

(In reply to :aceman [mostly away 23-29 Dec] from comment #39)
> 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?

"All this code" isn't actually much code if you deduct plenty of console.logs.

It is needed because we want to imitate details of nice and minimalist Windows Explorer multiple selection styling, hover, etc. To do that, it's necessary to know certain features of selected items, e.g.
- if they are first/last in a coherent selected block (e.g. last-in-block of selected must not have bottom-border),
- or if it's a selected item before the hovered item (selected item having bottom border, hovered item having top-border, then it looks like double-border, which is undesired).
CSS does not have a "previous sibling" selector, so anything which requires looking at the next item for formatting the current item needs special attributes which can then be used for direct css formatting.

Yes, this should ideally be implemented in some deeper layer, because we'd also want this for other listboxes like contacts sidebar. Maybe it could be implemented in toolkit's listbox.xml:
https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/listbox.xml
I wouldn't know if that's the right layer or if it should be even deeper, maybe in underlying C++ of XUL for better performance. But to be honest, any of those are deeper than I'm willing to go for a cosmetic change.

So for now, I think it's fine to just create our own marker attributes on the surface and then apply the desired CSS.
About the complexity of the CSS, I can't comment.

I think if we want to implement emitting these attributes at a deeper level, we can still do that later in another bug. Then we'd just remove some of our own listeners and on-event functions to set those attributes, which is easy to roll-back. CSS would probably remain the same.

> But if Paenglab thinks this is the right place and likes the appearance, I
> can review just the code as a last resort ;)

Please do :)

> But where are the speed improvements that we talked about? Where do the
> existing 'slow' functions change?

I haven't implemented the alleged performance improvements for routines of bug 663695 yet, and it's not required to land this bug, so I'll do that later if at all.

> Also please remove any observers you add:
> gAttachmentsFocusObserver.disconnect() in ComposeUnload().

Good catch, I'll add that in the next iteration.

Comment 42

[Thomas D. (:thomas8)]

Comment on attachment 8929613 [details] [diff] [review]
1412758_CSS.V3.patch

Getting better every time!

This seems to avoid all the double borders using the technique which I originally imagined. Now that I'm seeing it in action, I think my imagination wasn't strong enough... :)

We've created an interesting hover effect which makes the hover border appear bouncing like a rubber band. I double-checked with Windows Explorer and didn't find any bouncing, because they always conflate those borders.

If you're still willing, we should try to do the same, because whilst the bouncing is a very creative variation, it might also be considered shaky.
I'm not sure what is the best way of technical realization, but what about this:

- maybe first implement a slightly more generous spacing between items, as in contacts side bar, and as I suggested before in more detail (I'm not sure if this has been addressed or not, but I don't see it).

- then for hover and focus borders, only set top, left, and right borders. Instead of bottom border, you set top border of the next sibling item using css adjacent sibling selector with + operator (or ~ if that's better).
- Likewise for the border between selected items, that must be the top border of selected items except those which have selectedFirstOfBlock attribute.
- Possibly ensure through appropriate (asymetric?) padding that the line between two selected items looks centered, but also the composed focus/hover border must still look reasonably centered. Explorer somehow gets this quite right.

- Please address comment 37, I need a bit of space at top and bottom of list to create a visible dropmarker even when you've already drawn your focus/hover borders, which only works if my dropmarker increases line thickness for which I was thinking of css-outline which then needs a bit of space. Otherwise for dropmarkers at selection borders, I hope it'll work out if I use the now-unused bottom border of a focused/hovered item which then adds with the top border of the next item to a thicker border which shows the dropmarker. That's under the assumption that we want the dropmarker to have highlight color so it would otherwise be invisible as it blends in with focus/hover borders at the edge of selected items.

Something like that, I think you get the idea. Good luck! :)

Comment 43

[Thomas D. (:thomas8)]

Attachment: Markers-Patch V.4.1: For attachmentBucket onMouseOver, onSelect and 'currentItem-Change' events, add selection marker attributes for CSS styling

Updated markers patch, now less code as we've done a peripheral function name change elsewhere already.

(In reply to :aceman [mostly away 23-29 Dec] from comment #39)
> Comment on attachment 8928714 [details] [diff] [review]
> ...

I already answered most of your questions in comment 41.

> Also please remove any observers you add:
> gAttachmentsFocusObserver.disconnect() in ComposeUnload().

I tried that and was surprised to find that gLanguageObserver also doesn't disconnect, and that was added by Jörg who certainly wouldn't omit that if it was needed. According to specification, it looks as if disconnecting isn't required here:

> Note: According to specification a MutationObserver is deleted by the garbage
> collector if the target element is deleted.

Comment 44

[Richard Marti (:Paenglab)]

Attachment: 1412758_CSS.V4.patch

This should be the final patch.

Comment 45

[Thomas D. (:thomas8)]

Comment on attachment 8938887 [details] [diff] [review]
1412758_CSS.V4.patch

Best ever! :)
Thank you for removing the guitar effect.
I'm still more than happy that you are doing this because it looks like a lot of delicate work.

This is 97% perfect.
Unfortunately, you never commented on some of my proposals, so it's hard to tell if you forgot them, ignored them, don't like them, or they were technically too hard.

1) For drag and drop, I need an extra px of whitespace at the top of list, otherwise I don't know how to create a 2px dropmarker in highlight color without touching the top border. If I collapse my 1px highlight dropmarker with the 1px highlight item border, it's invisible. I thought we could do that here.

2) nth time, I think vertical height of each attachment is too small, and inconsistent as it's smaller than contacts side bar. I think it's just 1px more height to the bottom of each item which is needed, which stretches lists of attachments a bit to look more pleasant. Right now it looks a bit squeezed. Contacts sidebar item height looks better - can't we make them the same?

3) nth time, I think focus/hover border is very faint and hard to see. Why not use "highlight" as a color which in my tests was a bit brighter?

4) This one is new: 1 trivial glitch of adding up borders which should collapse:
Select item 1, Ctrl-select item 3, hold Ctrl+cursor up to move focus ring to item 2. Then hover over item 3. -> double border seen at the bottom of item 2.

5) Pls add ui-r=ThomasD to the patch.

Comment 46

[Richard Marti (:Paenglab)]

Attachment: 1412758_CSS.V4.1.patch

(In reply to Thomas D. (currently busy elsewhere) from comment #45)
> Comment on attachment 8938887 [details] [diff] [review]
> 1412758_CSS.V4.patch
> 
> This is 97% perfect.
> Unfortunately, you never commented on some of my proposals, so it's hard to
> tell if you forgot them, ignored them, don't like them, or they were
> technically too hard.

Sometimes hard to remember what you wrote with so much text.
 
> 1) For drag and drop, I need an extra px of whitespace at the top of list,
> otherwise I don't know how to create a 2px dropmarker in highlight color
> without touching the top border. If I collapse my 1px highlight dropmarker
> with the 1px highlight item border, it's invisible. I thought we could do
> that here.

This belongs to the DnD bug. I think, adding only space doesn't work. You'd need to add a dropmarker child in this listbox which uses this space. But again, this is not the bug for this.

> 2) nth time, I think vertical height of each attachment is too small, and
> inconsistent as it's smaller than contacts side bar. I think it's just 1px
> more height to the bottom of each item which is needed, which stretches
> lists of attachments a bit to look more pleasant. Right now it looks a bit
> squeezed. Contacts sidebar item height looks better - can't we make them the
> same?

I measured them and they have the same height.

> 3) nth time, I think focus/hover border is very faint and hard to see. Why
> not use "highlight" as a color which in my tests was a bit brighter?

No, it's the same as Explorer and treeitems. I don't change it.

> 4) This one is new: 1 trivial glitch of adding up borders which should
> collapse:
> Select item 1, Ctrl-select item 3, hold Ctrl+cursor up to move focus ring to
> item 2. Then hover over item 3. -> double border seen at the bottom of item
> 2.

Should be fixed.

> 5) Pls add ui-r=ThomasD to the patch.

Done.

Comment 47

[Thomas D. (:thomas8)]

Comment on attachment 8939233 [details] [diff] [review]
1412758_CSS.V4.1.patch

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

(In reply to Richard Marti (:Paenglab) from comment #46)
> Created attachment 8939233 [details] [diff] [review]
> 1412758_CSS.patch
> 
> (In reply to Thomas D. (currently busy elsewhere) from comment #45)
> > Comment on attachment 8938887 [details] [diff] [review]
> > 1412758_CSS.V4.patch
> > 
> > This is 97% perfect.
> > Unfortunately, you never commented on some of my proposals, so it's hard to
> > tell if you forgot them, ignored them, don't like them, or they were
> > technically too hard.
> 
> Sometimes hard to remember what you wrote with so much text.
>  
> > 1) For drag and drop, I need an extra px of whitespace at the top of list,
> > otherwise I don't know how to create a 2px dropmarker in highlight color
> > without touching the top border. If I collapse my 1px highlight dropmarker
> > with the 1px highlight item border, it's invisible. I thought we could do
> > that here.
> 
> This belongs to the DnD bug. 

OK.

> > 2) nth time, I think vertical height of each attachment is too small, and
> > inconsistent as it's smaller than contacts side bar. I think it's just 1px
> > more height to the bottom of each item which is needed, which stretches
> > lists of attachments a bit to look more pleasant. Right now it looks a bit
> > squeezed. Contacts sidebar item height looks better - can't we make them the
> > same?
> 
> I measured them and they have the same height.

I also measured, and at least on my trybuild 59.0a1 (2017-12-17) (32-bit), with your patch applied, a sequence of 8 selected items is significantly (like 8px) less high than before (release), and than the same number of items selected in contacts sidebar (with patch applied). I can actually see this with my bare eyes. My messengercompose.css and attachmentlist.css are exactly the same, so unless something could have changed or remained behind outside those files which is still affecting my local installation, there's a difference. Do you need a screenshot?

> > 3) nth time, I think focus/hover border is very faint and hard to see. Why
> > not use "highlight" as a color which in my tests was a bit brighter?
> 
> No, it's the same as Explorer and treeitems. I don't change it.

So why do we have "highlight" if not for highlighting things? Anyway, I'll just continue squeezing my eyes to spot that very faint focus marker, both in Win Explorer and here. Consistency is only as good as the original...

> > 4) This one is new: 1 trivial glitch of adding up borders which should
> > collapse:
> > Select item 1, Ctrl-select item 3, hold Ctrl+cursor up to move focus ring to
> > item 2. Then hover over item 3. -> double border seen at the bottom of item
> > 2.
> 
> Should be fixed.

Not fixed yet for me, following exactly my steps. Afasics, the rule you changed for this patch does NOT catch this.
You need a rule of this type:
((current AND (NOT selected)) + selected):hover --> no upper border
(hovering on a selected item where the immediate previous sibling is focused, but not selected)

I'm also wondering if the rule you changed for this patch could not be rewritten easier:

    #attachmentBucket:focus > attachmentitem[selectedBeforeCurrentItem] +
      attachmentitem[selected="true"],
      <snip> {
        border-top-color: var(--attachmentitem-selectedBeforeCurrentBorder);
      }

That's applicable to: a selected item which is immediately preceded by an item marked as (selected item before current item).
Iow: a (selected and current) item which is immediately preceded by a selected item. Wouldn't that be easier to write?

Also, you're not fully exploiting selectedBeforeCurrentItem=true as originally intended, which is why you end up with hover/current borders of different size, which isn't nice, and Windows Explorer doesn't do that either.

STR

- select item 1
- Ctrl+Cursor-down so that item 2 is focused, but not selected
(- for comparison, select two items and focus the second one, then loot at focus ring height)

Actual result:
- focus ring is smaller than usual, especially on top
(- when second of two selected items focused, it's bigger on top)

Expected:
- focus ring of usual size, using (I believe) the bottom border of the previous item, and omitting the top border of the focused item. Btw, can't we use outline-top for focused item so that we can add all visible borders remain associated with the item where they logically belong? The advantage of outline should be that it should also work on the first item where we can't manipulate any previous item because there's none. maybe with outline, we don't even need selected-before-hovered-item?
- add 1px of absolute height at the bottom of every item (not sure which property that is), to fix the "squeezed" layout and make focus border come out symmetrical.

> > 5) Pls add ui-r=ThomasD to the patch.
> 
> Done.

Thanks. Does it not need a comma before ui-r=...?

Comment 48

[Thomas D. (:thomas8)]

(In reply to Thomas D. (currently busy elsewhere) from comment #47)
> Comment on attachment 8939233 [details] [diff] [review]

> > > 4) This one is new: 1 trivial glitch of adding up borders which should
> > > collapse:
> > > Select item 1, Ctrl-select item 3, hold Ctrl+cursor up to move focus ring to
> > > item 2. Then hover over item 3. -> double border seen at the bottom of item
> > > 2.
> > 
> > Should be fixed.
> 
> Not fixed yet for me, following exactly my steps. Afasics, the rule you
> changed for this patch does NOT catch this.
> You need a rule of this type:
> ((current AND (NOT selected)) + selected):hover --> no upper border
> (hovering on a selected item where the immediate previous sibling is
> focused, but not selected)

In fact, couldn't we just radically simplify this by making it so that for hovered and focused items, we always use border-bottom, border-left, border-right, and outline-top (!)? Outline-top overlays the previous item, isn't it? So that's exactly what we want, always. Maybe we can eliminate a lot of rules with that, even some of our new markers?

Comment 49

[Thomas D. (:thomas8)]

(In reply to Thomas D. (currently busy elsewhere) from comment #48)

> In fact, couldn't we just radically simplify this by making it so that for
> hovered and focused items, we always use border-bottom, border-left,
> border-right, and outline-top (!)?

What I really mean (for the avoidance of doubt), is to do that on hovered OR focused (or hovered AND focused) items...

> Outline-top overlays the previous item,
> isn't it? So that's exactly what we want, always. Maybe we can eliminate a
> lot of rules with that, even some of our new markers?

Comment 50

[Richard Marti (:Paenglab)]

There exists no outline-top. Outline is always all four sides.

Comment 51

[Richard Marti (:Paenglab)]

Attachment: 1412758_CSS.V5.patch

(In reply to Thomas D. (currently busy elsewhere) from comment #47)
> Comment on attachment 8939233 [details] [diff] [review]
> 1412758_CSS.V4.1.patch
> 
> Review of attachment 8939233 [details] [diff] [review]:
> -----------------------------------------------------------------
> > I measured them and they have the same height.
> 
> I also measured, and at least on my trybuild 59.0a1 (2017-12-17) (32-bit),
> with your patch applied, a sequence of 8 selected items is significantly
> (like 8px) less high than before (release)

Something must have interfered here. Should be ok now.

> So why do we have "highlight" if not for highlighting things? Anyway, I'll
> just continue squeezing my eyes to spot that very faint focus marker, both
> in Win Explorer and here. Consistency is only as good as the original...

Ask Microsoft. ;)

> > Should be fixed.
> 
> Not fixed yet for me, following exactly my steps. Afasics, the rule you
> changed for this patch does NOT catch this.

But a other case you haven't seen. :)

All in all this is a very tricky thing as we need to check selected, current and hover in all combinations of the previous and following elements and your simplification wouldn't work on all combinations. But now it should work.

I also simplified the rules as the [selected="true"] before [selectedBefore... isn't needed because the [selectedBefore... already implies that the element is selected.

> Thanks. Does it not need a comma before ui-r=...?

Comment 52

[Richard Marti (:Paenglab)]

Oops, to early saved.

(In reply to Thomas D. (currently busy elsewhere) from comment #47)
> Thanks. Does it not need a comma before ui-r=...?

No

Comment 53

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #50)
> There exists no outline-top. Outline is always all four sides.

Who made that standard!? Crazy!
But there's always a way out:

Here's the trick for getting "outline-top" effect, using :before with  , position:relative; top:-1px to overlay the previous item. Works like a charme.

http://jsfiddle.net/ynefht11/3/

I'd be very interested to know if this works on listitems...
If it does, that looks like a very simple solution to just overlay whatever is there at the bottom border of the previous item. The charming part being that we restrict our formatting to the logical item where it really belongs.

(In reply to Richard Marti (:Paenglab) from comment #51)
> Created attachment 8940426 [details] [diff] [review]

I'm going to test that soon.
 
> But a other case you haven't seen. :)

You got me! Which one?
I'll ask Aceman to write testcases hehe...

> All in all this is a very tricky thing as we need to check selected, current
> and hover in all combinations of the previous and following elements and
> your simplification wouldn't work on all combinations.

these things are so confusing...
What I really really meant, is to do the 3-borders + "outline-top" trick on all of the following:
1) (hovered and selected) OR
2) (focused) -with or without selected- OR
3) (hovered AND selected AND focused) items... (probably covered by 1) or 2))

So that would be exactly one rule to cover all hover-selection and focus cases.
Which combination wouldn't work?

I can only think of two more rules required:
1) selected-last-of-block (to remove the bottom border normally separating selected items.
2) hovered (but not selected) (to change background-color on hover)
Anything else?

> But now it should work.

I'll try it, but I suspect we would be better of with the outline-top-mimick.

Comment 54

[Thomas D. (:thomas8)]

Btw, outline-top mimick trick found here:
https://stackoverflow.com/questions/12671898/outline-on-only-one-border

Comment 55

[Thomas D. (:thomas8)]

Comment on attachment 8940426 [details] [diff] [review]
1412758_CSS.V5.patch

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

Wow, wow, wow! Finally unsqueezed, and everything showing as it should. Nice.
I've got a few nits for the current version.

I think we should try the outline-top-mimick to simplify this even more. But we can also land this and try that in another bug.
I've got a few questions, forgive me if there's anything odd, I'm not claiming to fully understand this.

::: mail/themes/windows/mail/attachmentList.css
@@ +57,5 @@
> +    --attachmentitem-selectedBorder: var(--attachmentitem-selectedColor);
> +    --attachmentitem-selectedBottomBorder: rgb(204, 204, 204);
> +    --attachmentitem-selectedBackground: var(--attachmentitem-selectedColor);
> +    --attachmentitem-selectedImage: none;
> +    --attachmentitem-currentColor: rgb(125, 162, 206);

Interesting. Wish we'd also use that for Windows 10. Changing background color for selected and hovered items makes a lot more sense than creating another hover-border which is indistinguishable from keyboard focus border, whilst the two are fundamentally different.

@@ +59,5 @@
> +    --attachmentitem-selectedBackground: var(--attachmentitem-selectedColor);
> +    --attachmentitem-selectedImage: none;
> +    --attachmentitem-currentColor: rgb(125, 162, 206);
> +    --attachmentitem-selectedCurrentBorder: rgb(123, 195, 255);
> +    --attachmentitem-selectedBeforeCurrentBorder: var(--attachmentitem-selectedFocusBackground);

Rename pls, see below

@@ +60,5 @@
> +    --attachmentitem-selectedImage: none;
> +    --attachmentitem-currentColor: rgb(125, 162, 206);
> +    --attachmentitem-selectedCurrentBorder: rgb(123, 195, 255);
> +    --attachmentitem-selectedBeforeCurrentBorder: var(--attachmentitem-selectedFocusBackground);
> +    --attachmentitem-selectedLastCurrentBorder: var(--attachmentitem-selectedFocusBackground);

Rename pls, see below

@@ +85,3 @@
>      background-repeat: no-repeat;
>      background-size: 100% 100%;
> +    height: 1.83em;

Interesting. I wonder how they got that.

@@ +142,5 @@
> +  }
> +
> +  attachmentlist:focus > attachmentitem[current="true"] {
> +    border-color: var(--attachmentitem-selectedCurrentBorder);
> +    outline: none;

outline: Is this needed?

@@ +146,5 @@
> +    outline: none;
> +  }
> +
> +  attachmentlist:focus > attachmentitem[selected="true"][selectedLastOfBlock] {
> +    border-bottom-color: var(--attachmentitem-selectedLastCurrentBorder);

I think this variable should be renamed to --attachmentitem-selectedLastFocusBorder, because this stands for focus of bucket, and current stands for focus of item, but this isn't a focused item.

@@ +167,5 @@
> +    background-image: var(--attachmentitem-selectedFocusImage);
> +    outline: none;
> +  }
> +
> +  #attachmentBucket:focus > attachmentitem[selected="true"][selectedBeforeCurrentItem] {

If this is required to get the right specifity, it's fine. Otherwise consider dropping [selected="true"] because it's already included in [selectedBeforeCurrentItem]. This is one of the rules we could eliminate by using outline-mimic.

@@ +173,5 @@
>    }
>  
> +  @media (-moz-os-version: windows-win10) {
> +    #attachmentBucket:focus >
> +      attachmentitem:not([selectedLastOfBlock])[selectedBeforeHoveredItem],

What are these for?

@@ +182,5 @@
>  
> +    #attachmentBucket:focus > attachmentitem[current="true"] +
> +      attachmentitem[selected="true"]:hover,
> +    #attachmentBucket:focus > attachmentitem[selectedBeforeCurrentItem] +
> +      attachmentitem[selected="true"],

Could this be rewritten more readable?
    #attachmentBucket:focus > attachmentitem[selected="true"] +
      attachmentitem[selected="true"][current="true"],

@@ +185,5 @@
> +    #attachmentBucket:focus > attachmentitem[selectedBeforeCurrentItem] +
> +      attachmentitem[selected="true"],
> +    #attachmentBucket:focus > attachmentitem[selectedBeforeHoveredItem] +
> +      attachmentitem[selected="true"]:hover {
> +      border-top-color: var(--attachmentitem-selectedBeforeCurrentBorder);

This is confusing. The variable name suggests that this is a border for a selected item *before* a current(focused) item, no? But we're formatting the top border of focused item itself... Again, all these tricks could possibly be avoided with outline-mimick...

::: mail/themes/windows/mail/compose/messengercompose.css
@@ -20,5 @@
>    color: -moz-FieldText;
>  }
>  
> -#attachmentBucket:focus {
> -  box-shadow: 0 0 1px Highlight inset, 0 0 1px Highlight inset;

I realized that we would need this box-shadow when there are no attachments, i.e. we can't show any focus on an item, so we have to show it on the bucket. Any way in CSS that you could check for the existence of children and then format the parent?

Comment 56

[Richard Marti (:Paenglab)]

Attachment: 1412758_CSS V.5.1.patch

(In reply to Thomas D. (currently busy elsewhere) from comment #55)
> Comment on attachment 8940426 [details] [diff] [review]
> 1412758_CSS.V5.patch
> 
> Review of attachment 8940426 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Wow, wow, wow! Finally unsqueezed, and everything showing as it should. Nice.
> I've got a few nits for the current version.
> 
> I think we should try the outline-top-mimick to simplify this even more. But
> we can also land this and try that in another bug.
> I've got a few questions, forgive me if there's anything odd, I'm not
> claiming to fully understand this.

This doesn't simplify, it makes it more complex. You'd have to use the same rules for the top- and bottom-borders but add ::before or ::after plus the creation of this elements.

> ::: mail/themes/windows/mail/attachmentList.css
> @@ +57,5 @@
> > +    --attachmentitem-selectedBorder: var(--attachmentitem-selectedColor);
> > +    --attachmentitem-selectedBottomBorder: rgb(204, 204, 204);
> > +    --attachmentitem-selectedBackground: var(--attachmentitem-selectedColor);
> > +    --attachmentitem-selectedImage: none;
> > +    --attachmentitem-currentColor: rgb(125, 162, 206);
> 
> Interesting. Wish we'd also use that for Windows 10. Changing background
> color for selected and hovered items makes a lot more sense than creating
> another hover-border which is indistinguishable from keyboard focus border,
> whilst the two are fundamentally different.

This is for Windows 10. And we stay now to follow the Windows style.

> @@ +85,3 @@
> >      background-repeat: no-repeat;
> >      background-size: 100% 100%;
> > +    height: 1.83em;
> 
> Interesting. I wonder how they got that.

The treeitems use 1.8em. To get the 22px 1.83em was needed. I use now padding, it's simpler.

> @@ +142,5 @@
> > +  }
> > +
> > +  attachmentlist:focus > attachmentitem[current="true"] {
> > +    border-color: var(--attachmentitem-selectedCurrentBorder);
> > +    outline: none;
> 
> outline: Is this needed?

No

> @@ +146,5 @@
> > +    outline: none;
> > +  }
> > +
> > +  attachmentlist:focus > attachmentitem[selected="true"][selectedLastOfBlock] {
> > +    border-bottom-color: var(--attachmentitem-selectedLastCurrentBorder);
> 
> I think this variable should be renamed to
> --attachmentitem-selectedLastFocusBorder, because this stands for focus of
> bucket, and current stands for focus of item, but this isn't a focused item.

Yes, renamed.

> @@ +167,5 @@
> > +    background-image: var(--attachmentitem-selectedFocusImage);
> > +    outline: none;
> > +  }
> > +
> > +  #attachmentBucket:focus > attachmentitem[selected="true"][selectedBeforeCurrentItem] {
> 
> If this is required to get the right specifity, it's fine. Otherwise
> consider dropping [selected="true"] because it's already included in
> [selectedBeforeCurrentItem]. This is one of the rules we could eliminate by
> using outline-mimic.

It was overlooked by me. Outline-mimic makes nothing simpler, it's only a other way with more needed code, thus more complex.

> @@ +173,5 @@
> >    }
> >  
> > +  @media (-moz-os-version: windows-win10) {
> > +    #attachmentBucket:focus >
> > +      attachmentitem:not([selectedLastOfBlock])[selectedBeforeHoveredItem],
> 
> What are these for?

To show only one border between selected items.

> @@ +182,5 @@
> >  
> > +    #attachmentBucket:focus > attachmentitem[current="true"] +
> > +      attachmentitem[selected="true"]:hover,
> > +    #attachmentBucket:focus > attachmentitem[selectedBeforeCurrentItem] +
> > +      attachmentitem[selected="true"],
> 
> Could this be rewritten more readable?
>     #attachmentBucket:focus > attachmentitem[selected="true"] +
>       attachmentitem[selected="true"][current="true"],

No, but I use now :-moz-any()

> @@ +185,5 @@
> > +    #attachmentBucket:focus > attachmentitem[selectedBeforeCurrentItem] +
> > +      attachmentitem[selected="true"],
> > +    #attachmentBucket:focus > attachmentitem[selectedBeforeHoveredItem] +
> > +      attachmentitem[selected="true"]:hover {
> > +      border-top-color: var(--attachmentitem-selectedBeforeCurrentBorder);
> 
> This is confusing. The variable name suggests that this is a border for a
> selected item *before* a current(focused) item, no? But we're formatting the
> top border of focused item itself.

Using here now var(--attachmentitem-selectedFocusColor)

> ::: mail/themes/windows/mail/compose/messengercompose.css
> @@ -20,5 @@
> >    color: -moz-FieldText;
> >  }
> >  
> > -#attachmentBucket:focus {
> > -  box-shadow: 0 0 1px Highlight inset, 0 0 1px Highlight inset;
> 
> I realized that we would need this box-shadow when there are no attachments,
> i.e. we can't show any focus on an item, so we have to show it on the
> bucket. Any way in CSS that you could check for the existence of children
> and then format the parent?

This needs a attribute on attachmentBucket like [noAttachment] or simply [empty] set by JS. Then you could use:

#attachmentBucket[empty]:focus {
  box-shadow: 0 0 1px Highlight inset, 0 0 1px Highlight inset;
}

This would be something for bug 1427092 where you also show the close button depending if attachmentBucket is empty.

Comment 57

[Richard Marti (:Paenglab)]

Can we look to push this a bit? M-C is pushing to remove the -moz-border-*-colors. With this patches we do this also on this file.

Comment 58

[Richard Marti (:Paenglab)]

Attachment: 1412758_CSS.patch v.5.2

This one simplifies the border-color definition of the previous patch where I used border-top-color, border-right-color and border-left-color. Now I use only the shorcut border-color.

Comment 59

[:aceman]

Why do we need to implement this with so much code in the attachment list? Do other lists not need this? Is it because only this list does drag and drop of items?
Can I have a screenshot of what we achieve by this?

Comment 60

[Richard Marti (:Paenglab)]

Attachment: without-patch.png

It's not possible to make a good screenshot because it's the dynamic of hovering and selecting the items. Windows draws borders around the items and collapses them to one line. Without this code we end with two borders between two elements when one is selected and hovered.

The pre-patch code already removes this double border but only for selected items and doesn't honor hover or current. Also is the code optimized for Win 7 and Win 8 and 10 look wrong because of this. The patch also makes them look correct on this two.

This is how it looks without patch on Win 10 with border on all 4 sides around the items there should be only on the bottom.

Comment 61

[:aceman]

To me the new attachment looks the same as the other screenshots from Win 10 Explorer.

Comment 62

[Richard Marti (:Paenglab)]

The colors are too dark and on the sides there is a border which shouldn't be.

Comment 63

[Thomas D. (:thomas8)]

Comment on attachment 8944157 [details] [diff] [review]
1412758_CSS.patch v.5.2

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

Yes, thanks a lot, the styling behaviour is now nearing perfection, and code has been slimmed down.
Variables are still poorly named and confusingly nested (worse spread across OS variants so you never know if you're fixing one you might be killing the other), but I think we can leave that cleanup for another bug.

ui-r+ only for Windows 10 which I have seen on the try-build, and the recent slim-down color change was something from my list which looks correct from code inspection.

Comment 64

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #59)
> Why do we need to implement this with so much code in the attachment list?
> Do other lists not need this?

I'd say yes, they do. Richard?

> Can I have a screenshot of what we achieve by this?

We're trying to make the entire selecting/hovering/focusing behaviour mimic Windows Explorer exactly, and this has achieved a lot of polishing to that end. I can't vouch for the code which looks partially over-engineered, but I might be wrong because of the way this is spread across several OS who all fill some variables differently.

Comment 65

[Richard Marti (:Paenglab)]

(In reply to Thomas D. (currently busy elsewhere) from comment #64)
> (In reply to :aceman from comment #59)
> > Why do we need to implement this with so much code in the attachment list?
> > Do other lists not need this?
> 
> I'd say yes, they do. Richard?

Yes, but for Windows only.

Comment 66

[Richard Marti (:Paenglab)]

Comment on attachment 8944157 [details] [diff] [review]
1412758_CSS.patch v.5.2

Jörg, could you check this on Win 7?

Comment 67

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #65)
> (In reply to Thomas D. (currently busy elsewhere) from comment #64)
> > (In reply to :aceman from comment #59)
> > > Why do we need to implement this with so much code in the attachment list?
> > > Do other lists not need this?
> > 
> > I'd say yes, they do. Richard?
> 
> Yes, but for Windows only.

In that case this would go into listbox.xml so that all listboxes get the same behaviour. XUL was supposed to implement the platform-specifics so that widgets look native on all platforms.

Comment 68

[Jorg K (CEST = GMT+2)]

Looks like I'll have to review and land the CSS part first to get rid of -moz-border-*-colors now.

Comment 69

[Richard Marti (:Paenglab)]

In bug 1432251 I fixed this already. I'll upload then a rebased patch.

Comment 70

[Jorg K (CEST = GMT+2)]

OK, then you'll notice the bitrot due to https://hg.mozilla.org/comm-central/rev/26ee90059507#l4.12.

Comment 71

[Jorg K (CEST = GMT+2)]

Comment on attachment 8944157 [details] [diff] [review]
1412758_CSS.patch v.5.2

Needs refresh, clearing review for now.

Comment 72

[Richard Marti (:Paenglab)]

Attachment: 1412758_CSS.patch

Rebased patch.

Comment 73

[Jorg K (CEST = GMT+2)]

Attachment: Markers-Patch V.4.1: For attachmentBucket onMouseOver, onSelect and 'currentItem-Change' events, add selection marker attributes for CSS styling (rebased)

This needed to be rebased. We should move this bug forward one day :-)

Comment 74

[Jorg K (CEST = GMT+2)]

I've come to look at this now. Could a please have an executive summary of what this tries to do.

Between a current Daily and this patch I noticed that if the attachment bucket is focused and there multiple attachments are selected, the hover behaviour is different.

Daily: No indication what the currently hovered item is.
With patch: Hovered item is shown differently, like in Explorer.
Also see my comment #6.

I have the impression that the first item in the block is drawn darker, at least on Windows 7, which I can't see in Explorer.

Can you please enlighten me what these attributes are trying to achieve:
selectedBeforeCurrentItem, selectedLastOfBlock and selectedBeforeHoveredItem

I'd be happy with a small improvement of the current behaviour, that is, give an indication of what the hovered item is when a block is selected.

Oh I can see in the CSS patch, that different things happen on different versions of Windows. Coincidentally I built on a Win10 machine yesterday, I can look at it there too if I get my summary.

Comment 75

[Richard Marti (:Paenglab)]

The darker selected item is where the focus is. With CTRL UP/DOWN you can move this focus and then with SPACE select/deselect. With CTRL or SHIFT hold together with SPACE you can select a second item or all items between the selected and the one with focus. You shouldn't also see double borders between multiple selected items (is already without patch on Win 7).

Without patch, Win 10 uses the same appearance like Win 7 (except that it's flat) which is wrong on Win 10.

Comment 76

[Jorg K (CEST = GMT+2)]

Yes, thanks Richard, it works as you described. If the hover an item while changing the focus, other effects come into play.

I've looked at both patches again. The JS patch sets attributes selectedBeforeCurrentItem, selectedFirstOfBlock , selectedLastOfBlock and selectedBeforeHoveredItem. selectedFirstOfBlock is not used in the CSS.

These attributes are used to style borders. I think this is quite a bit of code burden for very little gain.

I'm happy to see a CSS patch only which will make it look more like the Explorer on Win7 and Win10, but without the special border tricks. Is that possible?

Comment 77

[:aceman]

Comment on attachment 8950193 [details] [diff] [review]
Markers-Patch V.4.1: For attachmentBucket onMouseOver, onSelect and 'currentItem-Change' events, add selection marker attributes for CSS styling (rebased)

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

So as we can't get this into XUL listbox, can we put this into mailWidgets.xml, attachmentlist-vertical binding?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1417,5 @@
> +function attachmentsMarkSelectedBeforeCurrentItem() {
> +  // Mark the selected item before the focused item.
> +  console.log ("attachmentsMarkSelectedBeforeCurrentItem()");
> +  let bucket = document.getElementById("attachmentBucket");
> +  let prevItem = bucket.currentItem.previousSibling;

This is a bit risky, assuming the previous attachment is really implemented as a sibling element. This is assuming things about internal implementation of the attachmentlist (a listbox I think). We should avoid that where possible. You probably want something like bucket.getItemAtIndex(bucket.currentIndex - 1).

@@ +1437,5 @@
> +  console.log ('**************************************************\n' +
> +    'attachmentBucket.onSelect --> set selection block markers');
> +
> +  // Remove marker attributes from previous selection, if they exist.
> +  let markedSelItems;

This variable seems only to be used inside the next loop.

@@ +1453,5 @@
> +  }
> +  let selItems = attachmentsSelectionGetSortedArray();
> +  selItems.forEach(item => {
> +    let previousItem = item.previousSibling;
> +    let nextItem = item.nextSibling;

Please no direct touching of siblings.

@@ +5692,5 @@
> +    oldMarkedItem.removeAttribute("selectedBeforeHoveredItem");
> +    console.log(oldMarkedItem.attachment.name + ' -> removeAttribute("selectedBeforeHoveredItem")');
> +  }
> +  if (target.tagName == "attachmentitem") {
> +    let prevItem = target.previousSibling;

All this code seems very similar to attachmentsMarkSelectedBeforeCurrentItem(). Can it be merged?

@@ +5694,5 @@
> +  }
> +  if (target.tagName == "attachmentitem") {
> +    let prevItem = target.previousSibling;
> +    if (prevItem && prevItem.selected) {
> +      prevItem.setAttribute("selectedBeforeHoveredItem","true");

Space after comma.

Comment 78

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+1) from comment #76)
> I'm happy to see a CSS patch only which will make it look more like the
> Explorer on Win7 and Win10, but without the special border tricks. Is that
> possible?
Can we do this in CSS only without the border styling (and without the JS part)?

Comment 79

[Richard Marti (:Paenglab)]

I try to look tomorrow into it.

Comment 80

[Richard Marti (:Paenglab)]

Attachment: CSS only patch

This is a CSS only patch. It makes all what is doable through CSS only (I think).

When you have four attachments and the first and the third are selected and you now move the focus with CTRL-up/down, you can see a jumping borderbetween the not selected and selected items (like a guitar string). I think this is not the end of the world and we can live with it. With this patch, especially on Win 10, it looks more native than before.

Comment 81

[Jorg K (CEST = GMT+2)]

Comment on attachment 8951996 [details] [diff] [review]
CSS only patch

This looks fine on Win7, I'll try it on Win10 next.

Comment 82

[Jorg K (CEST = GMT+2)]

Comment on attachment 8945097 [details] [diff] [review]
1412758_CSS.patch

I preference would be to go with a CSS patch here only, even if it's only 99.37% optimal. So clearing the review request.

Comment 83

[Jorg K (CEST = GMT+2)]

Comment on attachment 8951996 [details] [diff] [review]
CSS only patch

This doesn't really mimic Win10. In the Explorer on Win10, I don't see any borders for selected items. Only hovering an already selected item gives a border. All selected items have the same background.

The patch has borders for selected items and hovered items have a darker background. Both these aren't correct on Win10.

Comment 84

[Jorg K (CEST = GMT+2)]

Attachment: 1412758.png - JK's Win10

Hmm, looking at the attached screenshots, Thomas' looks different. His show borders, mine doesn't.

I forgot to mention the focus ring around the focussed item, last one in the block.

Comment 85

[Jorg K (CEST = GMT+2)]

Comment on attachment 8951996 [details] [diff] [review]
CSS only patch

Clearing the review until we decide which theme of Win10 we want to follow. Apparently the patch tries to follow the theme visible in Thomas' screenshots whereas I see a different theme. Maybe the theme has changed, I'm on Win10 1709.

This just goes to show that we shouldn't invest complicated code to follow something that is constantly changing.

Comment 86

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+1) from comment #83)
> Comment on attachment 8951996 [details] [diff] [review]
> CSS only patch
> 
> This doesn't really mimic Win10. In the Explorer on Win10, I don't see any
> borders for selected items. Only hovering an already selected item gives a
> border. All selected items have the same background.

Multiple selected items have no border at the sides, only between them. The one with the focus is also darker or not.

> The patch has borders for selected items and hovered items have a darker
> background. Both these aren't correct on Win10.

What is not correct?

(In reply to Jorg K (GMT+1) from comment #84)
> Created attachment 8952037 [details]
> 1412758.png - JK's Win10
> 
> Hmm, looking at the attached screenshots, Thomas' looks different. His show
> borders, mine doesn't.

You should see a darker line between the items.

> I forgot to mention the focus ring around the focussed item, last one in the
> block.

This one is weird. In Explorer I have no such focus ring.

Comment 87

[Richard Marti (:Paenglab)]

Attachment: Work-Win10.png

How it look on my plain work PC. The last selected has the focus and below is one hovered item.

Comment 88

[Jorg K (CEST = GMT+2)]

Please look at my screenshot in attachment 8952037 [details].

(In reply to Richard Marti (:Paenglab) from comment #86)
> Multiple selected items have no border at the sides, only between them. The
> one with the focus is also darker or not.
Not for me: No lines in between and nothing is darker.

> You should see a darker line between the items.
I don't.

> > I forgot to mention the focus ring around the focussed item, last one in the
> > block.
> This one is weird. In Explorer I have no such focus ring.
Well, I have it.

So, do I have a special theme that's different? I use "accent colour", does that make a difference?

Comment 89

[Richard Marti (:Paenglab)]

The accent colour shouldn't matter.

Comment 90

[Jorg K (CEST = GMT+2)]

I did all sorts of things: Switched off Clover (Explorer helper), switched off Classic Explorer, switched theme to "Windows", switched off accent colour. All made no difference:
- No lines between items in a selected block
- Hovered item has a border, but no darker background
- Focused item (can be changed with Ctrl+Up/Down Arrow) has a focus ring.

I'll check another Win10 PC in the family.

Comment 91

[Richard Marti (:Paenglab)]

The darkening can be an error by me (remnant of Win 7 styles). Need to check this on my PC.

Comment 92

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #91)
> The darkening can be an error by me (remnant of Win 7 styles).
Right. The family Win10 PC has:
- YES, lines between items in a selected block
- Hovered item has a border, but *NO* darker background
- Focused item (can be changed with Ctrl+Up/Down Arrow) DOES NOT HAVE a focus ring.
So the family PC behaves like is shown in Thomas' and Richard's screenshots.

Comment 93

[Jorg K (CEST = GMT+2)]

BTW, the hunk in mail/themes/windows/mail/compose/messengercompose.css
#attachmentBucket:focus {	
  box-shadow: 0 0 1px Highlight inset, 0 0 1px Highlight inset;	
}
is no longer required?

Comment 94

[Jorg K (CEST = GMT+2)]

Comment on attachment 8950193 [details] [diff] [review]
Markers-Patch V.4.1: For attachmentBucket onMouseOver, onSelect and 'currentItem-Change' events, add selection marker attributes for CSS styling (rebased)

Every time I look at this, I think that it's an awful lot of code with mutation observers, mouse-over observers and making DOM changes just to get some borders a tiny little bit more native on one platform (which in itself has various variations). I think it's over engineered.

Comment 95

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+1) from comment #93)
> BTW, the hunk in mail/themes/windows/mail/compose/messengercompose.css
> #attachmentBucket:focus {	
>   box-shadow: 0 0 1px Highlight inset, 0 0 1px Highlight inset;	
> }
> is no longer required?

I thought, I leave it in, so that bug 1428631 can change it to show only when empty.

Comment 96

[Richard Marti (:Paenglab)]

Attachment: CSS only patch [landed in comment #100]

Fixed the focused/hover on selected item background colour.

Comment 97

[Jorg K (CEST = GMT+2)]

Comment on attachment 8952214 [details] [diff] [review]
CSS only patch [landed in comment #100]

As I said before, my Win10 machine behaves differently than the family Win10 machine, so I'm not able to check this 100%. I believe it works now like I have seen Explorer behave elsewhere.

Personally I think the hover border is very very subtle and hard to see since the items are separated by lines (not on my machine). But if M$ wanted it like that, let's go with that.

Needless to say that there are other Windows themes, like the high contrast ones. With a high contrast theme on Win10 and this patch, I don't see any hovering, neither on selected nor unselected items. At least on this machine here, Explorer hovers unselected items with a border.

So you might want to revise the patch again. If not, r=jorgk.

Comment 98

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+1) from comment #97)
> Comment on attachment 8952214 [details] [diff] [review]
> CSS only patch
> 
> Needless to say that there are other Windows themes, like the high contrast
> ones. With a high contrast theme on Win10 and this patch, I don't see any
> hovering, neither on selected nor unselected items. At least on this machine
> here, Explorer hovers unselected items with a border.
> 
> So you might want to revise the patch again. If not, r=jorgk.

Win 7 Classic and High Contrast Explorer shows also no hover. So we are consistent -> no change planned.

Comment 99

[Jorg K (CEST = GMT+2)]

Comment on attachment 8952214 [details] [diff] [review]
CSS only patch [landed in comment #100]

OK, I've just confirmed on the family Win10 PC that high contrast themes do hover with a border in Explorer. So TB is really not consistent with the Explorer in that case.

Comment 100

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/91311b1d6aa3
Improve the appearance of selected attachment items on Windows. r=jorgk