Closed Bug 1472555 Opened 6 years ago Closed 6 years ago

Remove the "listbox" element and related bindings

Categories

(Toolkit :: UI Widgets, task, P3)

Product:
Toolkit
Component:
UI Widgets
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(5 files, 3 obsolete files)

Bug 1472555 - Part 1 - Remove listbox crashtests and reftests.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1472555 - Part 2 - Convert DOM tests to use the "richlistbox" element.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1472555 - Part 3 - Convert Toolkit tests to use the "richlistbox" element.
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1472555 - Part 4 - Remove the "listbox" bindings.
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1472555 - Part 5 - Remove the listbox layout.
59 bytes, text/x-review-board-request
bzbarsky
: review+
surkov
: review+
Details 
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".
Blocks: 1472556
Blocks: 1472558
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 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)
Depends on: 1474108
Depends on: 1474258
Attachment #8990498 - Flags: feedback?(bzbarsky)
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 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 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+
(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.
Attachment #8990498 - Flags: feedback?(bzbarsky)
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 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?
(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 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 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 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 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 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 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+
(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 :-(
(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.
(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)
(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 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+
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
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
We'll be ready with the listbox removal in bug 1475817 within 24 hours :-)
Depends on: 1381706
Attachment #8990487 - Attachment is obsolete: true
Attachment #8990498 - Attachment is obsolete: true
Attachment #8990528 - Attachment is obsolete: true
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
Blocks: 1476611
Blocks: 1476639
Blocks: 1476659
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
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: