Closed Bug 1562994 Opened 5 months ago Closed 3 months ago

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

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 70.0

People

(Reporter: aleca, Assigned: aleca)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch 1562994-textbox-html-input.patch (obsolete) — Splinter Review
Assignee: nobody → alessandro
Status: NEW → ASSIGNED
Attachment #9082802 - Flags: review?(mkmelin+mozilla)
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
Attachment #9082802 - Flags: review?(mkmelin+mozilla)
Mentor: alessandro
Attached patch 1562994-textbox-html-input.patch (obsolete) — Splinter Review

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

Attachment #9082802 - Attachment is obsolete: true
Attachment #9083166 - Flags: review?(mkmelin+mozilla)
Attached patch 1562994-textbox-html-input.patch (obsolete) — Splinter Review

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

Attachment #9083166 - Attachment is obsolete: true
Attachment #9083166 - Flags: review?(mkmelin+mozilla)
Attachment #9083167 - Flags: review?(mkmelin+mozilla)
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
Attachment #9083167 - Flags: review?(mkmelin+mozilla) → feedback+
Attached patch 1562994-textbox-html-input.patch (obsolete) — Splinter Review
Attachment #9083167 - Attachment is obsolete: true
Attachment #9083400 - Flags: review?(philipp)
Attachment #9083400 - Flags: feedback+
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.
Attachment #9083400 - Flags: review?(philipp) → review+

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

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

Attachment #9083400 - Attachment is obsolete: true
Attachment #9083504 - Flags: review+

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

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED

This is really a Calendar bug, but anyway.

Target Milestone: --- → Thunderbird 70.0
You need to log in before you can comment on or make changes to this bug.