Remove the "listbox" element and related bindings

RESOLVED FIXED in Firefox 63

Status

()

task
P3
normal
RESOLVED FIXED
Last year
13 days ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks 3 bugs)

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(5 attachments, 3 obsolete attachments)

Assignee

Description

Last year
Once all uses of "listbox" are converted to "richlistbox" in bug 1472554, we can remove most of the listbox-related bindings, as well as the platform code that supports the "listbox" element and is not required by "richlistbox".
Assignee

Updated

Last year
Blocks: 1472556
Assignee

Updated

Last year
Blocks: 1472558
Comment hidden (mozreview-request)
Assignee

Comment 2

Last year
This is just the beginning of the "listbox" element removal. I looked at the original crashtest bugs one by one, and these looked safe to remove at first glance. There are some tests that use <listbox> for testing grid layout, I'm not sure if they can be remove straight away, so I've left them out of this patch, and I'll ask for review separately.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 6

Last year
Comment on attachment 8990498 [details]
Bug 1472555 - Part 3 - Tentatively remove remaining listbox crashtests.

These are the four cases for which, from the original bug description, it looked like there might be some value left in the tests. Still, I'm not sure if it's worth spending time converting them, and if so which parts should be kept. Considering we're actively tearing down XUL and grid layout, it may be easier to just remove these as well.
Attachment #8990498 - Flags: feedback?(bzbarsky)
Assignee

Updated

Last year
Depends on: 1474108
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

11 months ago
Depends on: 1474258
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

11 months ago
Attachment #8990498 - Flags: feedback?(bzbarsky)
Assignee

Comment 20

11 months ago
With the removal of the bindings, we can start to see if there are any tests that I missed and still depend on "listbox":

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f6bea109690128e497fa7c0410485be45419644

Comment 21

11 months ago
mozreview-review
Comment on attachment 8990695 [details]
Bug 1472555 - Part 4 - Remove the "listbox" bindings.

https://reviewboard.mozilla.org/r/255782/#review262656

This is great, thanks!

::: toolkit/content/widgets/listbox.xml:638
(Diff revision 1)
> -      ]]>
> -      </handler>
> -    </handlers>
> -  </binding>
> -
>    <binding id="listitem"

If we don't bind this to any elements anymore and we are deleting one of two bindings that extend it (listitem-checkbox), then could we fold this implementation into the remaining binding that extends it (richlistitem) and then delete this one?
Attachment #8990695 - Flags: review?(bgrinstead) → review+

Comment 22

11 months ago
mozreview-review
Comment on attachment 8990530 [details]
Bug 1472555 - Part 3 - Convert Toolkit tests to use the "richlistbox" element.

https://reviewboard.mozilla.org/r/255600/#review262670
Attachment #8990530 - Flags: review?(bgrinstead) → review+
Assignee

Comment 23

11 months ago
(In reply to Brian Grinstead [:bgrins] from comment #21)
> If we don't bind this to any elements anymore and we are deleting one of two
> bindings that extend it (listitem-checkbox), then could we fold this
> implementation into the remaining binding that extends it (richlistitem) and
> then delete this one?

Exactly.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

11 months ago
Attachment #8990498 - Flags: feedback?(bzbarsky)
Assignee

Comment 33

11 months ago
The last patch removes about 3,100 lines of code. The work in bug 1465135 and bug 1465141 is also absorbed here.
Blocks: 1465135, 1465141

Comment 34

11 months ago
mozreview-review
Comment on attachment 8991999 [details]
Bug 1472555 - Part 5 - Remove the listbox layout.

https://reviewboard.mozilla.org/r/256886/#review263780

is there a such thing as mutli column listboxes?

::: accessible/base/ARIAMap.cpp
(Diff revision 1)
> -    eNoValue,
> -    eNoAction, // XXX: should depend on state, parent accessible
> -    eNoLiveAttr,
> -    kGenericAccType,
> -    states::READONLY
> -  },

these ARIA ones, and not related with XUL listboxes

::: accessible/base/XULMap.h:14
(Diff revision 1)
>  XULMAP_TYPE(dropMarker, XULDropmarkerAccessible)
>  XULMAP_TYPE(editor, OuterDocAccessible)
>  XULMAP_TYPE(findbar, XULToolbarAccessible)
>  XULMAP_TYPE(groupbox, XULGroupboxAccessible)
>  XULMAP_TYPE(iframe, OuterDocAccessible)
> -XULMAP_TYPE(listbox, XULListboxAccessibleWrap)
> +XULMAP_TYPE(listheader, XULColumAccessible)

it seems like unrelated change, what listheader is used for?
Assignee

Comment 35

11 months ago
(In reply to alexander :surkov from comment #34)
> is there a such thing as mutli column listboxes?
> it seems like unrelated change, what listheader is used for?

Apparently, there used to be multi-column listboxes. The "listbox" element used to be structured like this:

<listbox>
  <listhead>
    <listheader/>
    <listheader/>
  </listhead>
  (
    scrolling content:
    <listitem/>
  )
</listbox>

When "richlistbox" was created it was not meant to have more than one column, so the binding used to be structured like this:

<richlistbox>
  <listheader/>
  (
    scrolling content:
    <richlistitem/>
  )
</richlistbox>

At some point, however, there was the need for what looks like a multi-column "richlistbox". In particular, we use such an arrangement for the handler application preferences. Unfortunately, the only element that was considered unscrollable by "richlistbox" was "listheader", so the common practice became this one:

<richlistbox>
  <listheader>
    <treecol/>
    <treecol/>
  </listheader>
  (
    scrolling content:
    <richlistitem/>
  )
</richlistbox>

Basically, this changed the meaning of "listheader" to be similar to what "listhead" used to be. Does it make sense to change the accessible type on it? Today this arrangement is only used in Preferences and I'm not sure if there is anything to do to make those views more accessible, or if changing the accessible object makes any difference.
Flags: needinfo?(surkov.alexander)
Jorg, you may want to keep an eye on this; there are lots of listboxes in comm-central.

Comment 37

11 months ago
mozreview-review
Comment on attachment 8990487 [details]
Bug 1472555 - Part 1 - Remove most listbox crashtests.

https://reviewboard.mozilla.org/r/255558/#review263864

r=me, though to just so I'm clear: we could have left the tests; they just wouldn't be testing anything useful anymore, right?
Attachment #8990487 - Flags: review?(bzbarsky) → review+

Comment 38

11 months ago
mozreview-review
Comment on attachment 8990497 [details]
Bug 1472555 - Part 1 - Remove listbox crashtests and reftests.

https://reviewboard.mozilla.org/r/255566/#review263866
Attachment #8990497 - Flags: review?(bzbarsky) → review+

Comment 39

11 months ago
mozreview-review
Comment on attachment 8990498 [details]
Bug 1472555 - Part 3 - Tentatively remove remaining listbox crashtests.

https://reviewboard.mozilla.org/r/255568/#review263868

r=me.  I agree that trying to figure out how to modify these testcases is likely not worth it.
Attachment #8990498 - Flags: review+
Attachment #8990498 - Flags: feedback?(bzbarsky) → feedback+

Comment 40

11 months ago
mozreview-review
Comment on attachment 8990528 [details]
Bug 1472555 - Part 4 - Remove XBL crashtests for listbox.

https://reviewboard.mozilla.org/r/255596/#review263870
Attachment #8990528 - Flags: review?(bzbarsky) → review+

Comment 41

11 months ago
mozreview-review
Comment on attachment 8990529 [details]
Bug 1472555 - Part 2 - Convert DOM tests to use the "richlistbox" element.

https://reviewboard.mozilla.org/r/255598/#review263872
Attachment #8990529 - Flags: review?(bzbarsky) → review+

Comment 42

11 months ago
mozreview-review
Comment on attachment 8991999 [details]
Bug 1472555 - Part 5 - Remove the listbox layout.

https://reviewboard.mozilla.org/r/256886/#review263874

r=me on the non-accessible/ parts.  So nice to see the hackery go away in the frame constructor!
Attachment #8991999 - Flags: review?(bzbarsky) → review+

Comment 43

11 months ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #36)
> Jorg, you may want to keep an eye on this; there are lots of listboxes in
> comm-central.
Thanks Boris, we though we had it covered in bug 1472556, but apparently not :-(
Assignee

Comment 44

11 months ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #37)
> r=me, though to just so I'm clear: we could have left the tests; they just
> wouldn't be testing anything useful anymore, right?

Yeah, I think this is how crashtests work... :-/

I'll fold the crashtest and reftest removals into one changeset now that they are reviewed.

Comment 45

11 months ago
(In reply to :Paolo Amadini from comment #35)
> Apparently, there used to be multi-column listboxes.

Pardon the redundant clarification, but I just want to be super sure we aren't regressing accessibility here. Does this mean there were no multi-column listboxes converted to richlistbox in bug 1472554 and friends?
Flags: needinfo?(paolo.mozmail)
Assignee

Comment 46

11 months ago
(In reply to James Teh [:Jamie] from comment #45)
> Pardon the redundant clarification, but I just want to be super sure we
> aren't regressing accessibility here. Does this mean there were no
> multi-column listboxes converted to richlistbox in bug 1472554 and friends?

Yes, that's correct. The "richlistbox" element never supported the multi-column grid layout, which is a legacy of when "listbox" was used for views with configurable columns, like an operating system's file details view. We have only a few of these views remaining, scheduled for deletion, and we use "tree" for them. In the future, for history and bookmarks, but also lists in general, we want to provide some visual hierarchy, and new designs often have a "card" type layout that provides it better than a flat table.

There are a few examples in Preferences that look visually like a two-column grid, although functionally they aren't. In particular you can check the list of handler applications. It would make no difference if the file type and the menu where you can choose the associated application were displayed as two rows instead of side by side, and the layout is not customizable.

As I noted in comment 25, I'm not sure if there is anything to do to make those Preferences views more accessible, anyways this is the existing state and not regressed by the work here, so I'd leave any adjustment for a follow-up.

As an aside, we may also want to change how the list headers work, they are currently a child of the "richlistbox" but are outside of the anonymous "scrollbox". If we can make them either part of the scrollable area and fixed position, or not part of the "richlistbox" in the first place, it would allow us to remove the "scrollbox" element and simplify the conversion to Custom Elements.
Flags: needinfo?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #35)
> (In reply to alexander :surkov from comment #34)
> > is there a such thing as mutli column listboxes?
> > it seems like unrelated change, what listheader is used for?
> 
> Apparently, there used to be multi-column listboxes. The "listbox" element
> used to be structured like this:

> At some point, however, there was the need for what looks like a
> multi-column "richlistbox". In particular, we use such an arrangement for
> the handler application preferences. Unfortunately, the only element that
> was considered unscrollable by "richlistbox" was "listheader", so the common
> practice became this one:
> 
> <richlistbox>
>   <listheader>
>     <treecol/>
>     <treecol/>
>   </listheader>
>   (
>     scrolling content:
>     <richlistitem/>
>   )
> </richlistbox>

It looks like some accessibility features are technically missed for this case. AT navigates table/grid-like structures by cells and can retrieve headers associated with a given cell. However if multiple column richlistboxes are only used to create a specific placement, and won't be ever used for multi column list controls in the future, then it's ok. However if we aren't going to get rid XUL at all one day, then it leaves us with a breach to break user experience.

> Basically, this changed the meaning of "listheader" to be similar to what
> "listhead" used to be. Does it make sense to change the accessible type on
> it? Today this arrangement is only used in Preferences and I'm not sure if
> there is anything to do to make those views more accessible, or if changing
> the accessible object makes any difference.

yes, it makes sense now. thank you
Flags: needinfo?(surkov.alexander)

Comment 48

11 months ago
mozreview-review
Comment on attachment 8991999 [details]
Bug 1472555 - Part 5 - Remove the listbox layout.

https://reviewboard.mozilla.org/r/256886/#review264022

::: accessible/base/XULMap.h:14
(Diff revision 1)
>  XULMAP_TYPE(dropMarker, XULDropmarkerAccessible)
>  XULMAP_TYPE(editor, OuterDocAccessible)
>  XULMAP_TYPE(findbar, XULToolbarAccessible)
>  XULMAP_TYPE(groupbox, XULGroupboxAccessible)
>  XULMAP_TYPE(iframe, OuterDocAccessible)
> -XULMAP_TYPE(listbox, XULListboxAccessibleWrap)
> +XULMAP_TYPE(listheader, XULColumAccessible)

now I see it makes sense, however please update comment for XULColumAccessible class, and for XULColumnItemAccessible if xul:listcol are not in use any longer
Attachment #8991999 - Flags: review?(surkov.alexander) → review+
Assignee

Comment 49

11 months ago
Tryserver build for the folded patch queue with comments updated:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fdec4a260755b4c8d4543aab9d51ed31ff714f0
(In reply to :Paolo Amadini from comment #49)
> Tryserver build for the folded patch queue with comments updated:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0fdec4a260755b4c8d4543aab9d51ed31ff714f0

FYI Decision tasks are failing right now (Bug 1475656) so you'll likely need to re-push once that's fixed
Assignee

Comment 51

11 months ago
Due to unified compilation I had to sprinkle some #include and typedefs here and there after the rebase, let's try again:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41eefee834c45f84f932744f2b083d97061e413c

Comment 52

11 months ago
We'll be ready with the listbox removal in bug 1475817 within 24 hours :-)
Assignee

Updated

11 months ago
Depends on: 1381706
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

11 months ago
Attachment #8990487 - Attachment is obsolete: true
Assignee

Updated

11 months ago
Attachment #8990498 - Attachment is obsolete: true
Assignee

Updated

11 months ago
Attachment #8990528 - Attachment is obsolete: true
Assignee

Comment 58

11 months ago
This re-added the NativeName override on XULListitemAccessible since it bypassed XULMenuitemAccessible.

Tryserver build, rebased with the fix for bug 1381706:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad301d807484601ba0f99190e444224a7243b65e
Assignee

Updated

11 months ago
Blocks: 1476611
Assignee

Updated

11 months ago
Blocks: 1476639
Assignee

Updated

11 months ago
Blocks: 1476659

Comment 59

11 months ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d96a7724d63
Part 1 - Remove listbox crashtests and reftests. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c45f8a8b370
Part 2 - Convert DOM tests to use the "richlistbox" element. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ffebfb2053
Part 3 - Convert Toolkit tests to use the "richlistbox" element. r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/501544bfc95b
Part 4 - Remove the "listbox" bindings. r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/38ebcdae9bca
Part 5 - Remove the listbox layout. r=bz,surkov
Assignee: nobody → paolo.mozmail

Updated

13 days ago
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.