1562994 - Use HTML input instead of XUL textbox in calendar-item-bindings.js

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Alessandro Castellani [:aleca]]

https://searchfox.org/comm-central/rev/94d2c771f935e9402ca3d19a43fafeeaa8f0ac4f/calendar/base/content/calendar-item-bindings.js#23

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: 1562994-textbox-html-input.patch

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9082802 [details] [diff] [review]
1562994-textbox-html-input.patch

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

Hmm, it looks like calendar-item-date should really be <html:input is="calendar-item-date-input"> instead.
Sorry to expand the scope, but it should be an easy enough conversion and the css would be different.

::: calendar/base/content/calendar-bindings.css
@@ +17,5 @@
> +}
> +
> +.input-container > html|input {
> +  flex-grow: 1;
> +}
\ No newline at end of file

\ No newline at end of file

calendar-bindings.css is meant for bindings, and should be removed fairly soon, so not a good place to put this

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: 1562994-textbox-html-input.patch

No worries, this was easy to convert.
It actually simplifies the fields removing the necessity of handling inheritance nor adding extra CSS.

Comment 4

[Alessandro Castellani [:aleca]]

Attachment: 1562994-textbox-html-input.patch

Sorry, I had a leftover in the calendar bindings file.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9083167 [details] [diff] [review]
1562994-textbox-html-input.patch

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

Thx for updating!
Thinking about this still one bit further, I do notice it's quite questionable why it should be an input to begin with... It's really a time display label. (Textbox probably due to some accessibility/selection concerns?.)
Compare to e.g. the dateLabel in mail header toolbar. But, maybe we'll leave this for now...

Maybe someone from calendar want so sing off on this?

::: calendar/base/content/calendar-item-bindings.js
@@ +15,3 @@
>      connectedCallback() {
> +        this.classList.add("selectable-label", "plain");
> +        this.setAttribute("readonly", "true");

boolean attribute in html, so this should be
this.setAttribute("readonly", "readonly");

@@ +74,5 @@
>          return this.mItem;
>      }
>  }
>  
> +customElements.define("calendar-item-date", MozCalendarItemDate, { "extends": "input" });

for the CEs inheriting other elements, I think it's a good convention to add the -<parent> to the name.  we've been fairly consistent with this (but not fully, should clean up later). so calendar-item-date-input

Comment 6

[Alessandro Castellani [:aleca]]

Attachment: 1562994-textbox-html-input.patch

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9083400 [details] [diff] [review]
1562994-textbox-html-input.patch

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

::: calendar/base/content/calendar-task-view.xul
@@ +189,5 @@
>                    <label id="task-start-row-label"
>                           class="headline"
>                           value="&calendar.task-details.start.label;"
>                           align="end"/>
> +                  <html:input is="calendar-item-date-input" id="task-start-date" mode="start"/>

While we are changing this it might make sense to make sure this works well for accessibility, i.e. make sure the label is connected to the input. I'm not sure if `for="task-start-row-label"` would work on the input element given one is XUL and the other is html, but worth a try.

Comment 8

[Magnus Melin [:mkmelin]]

xref bug 1513323.

Comment 9

[Alessandro Castellani [:aleca]]

Yeah, the for attribute doesn't work but the control does.

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: 1562994-textbox-html-input.patch

Updated the patch and lunched a try run just to be sure, even if this is a minor change.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ef243e4ab5d043483146c01c202ef0fe10b11761

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f7afb221aa10
textbox to html input in calendar-item-bindings.js. r=mkmelin,philipp

Comment 12

[Jorg K (CEST = GMT+2)]

This is really a Calendar bug, but anyway.