Closed Bug 1563000 Opened 2 years ago Closed 2 years ago

Use HTML input instead of XUL textbox in calendar/base/content/dialogs

Categories

(Thunderbird :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 70.0

People

(Reporter: aleca, Assigned: aleca)

References

Details

Attachments

(1 file, 5 obsolete files)

  • calendar-print-dialog.xul
  • calendar-properties-dialog.xul
  • calendar-subscriptions-dialog.xul
  • calendar-summary-dialog.xul
Assignee: nobody → alessandro
Mentor: alessandro
Status: NEW → ASSIGNED
Attached patch 1563000-textbox-html-input.patch (obsolete) — Splinter Review
Attachment #9084466 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9084466 [details] [diff] [review]
1563000-textbox-html-input.patch

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

input-fields.css doesn't exist. Does this depend on some other bug?

::: calendar/base/content/dialogs/calendar-properties-dialog.xul
@@ +79,5 @@
>                 control="calendar-uri"/>
>          <!-- XXX Make location field readonly until Bug 315307 is fixed -->
> +        <html:input id="calendar-uri"
> +                    class="calendar-input"
> +                    readonly="true"

readonly is a boolean attribute, so readonly="readonly"

::: calendar/base/content/dialogs/calendar-summary-dialog.xul
@@ +144,5 @@
>            <row align="center">
> +            <label value="&read.only.title.label;" control="item-title"/>
> +            <html:input id="item-title"
> +                        class="selectable-label plain calendar-input"
> +                        readonly="true"/>

same here and for all the other readonlys
Attached patch 1563000-textbox-html-input.patch (obsolete) — Splinter Review

Sorry, I forgot to hg add the new file.

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

Unbitrotted from trunk.

Heads up that the .calendar-input-container class is currently not used, but it'll be necessary once the the grid and row elements get removed.

Is it OK to leave it in since the de-grid tasks will immediately follow this one, or should we remove it and let khushil324 add it when needed?

Attachment #9084811 - Attachment is obsolete: true
Attachment #9084811 - Flags: review?(mkmelin+mozilla)
Flags: needinfo?(khushil324)
Attachment #9085132 - Flags: review?(mkmelin+mozilla)

(In reply to Alessandro Castellani (:aleca) from comment #4)

Is it OK to leave it in since the de-grid tasks will immediately follow this one, or should we remove it and let khushil324 add it when needed?

Keep it now. I will take care of this in the de-grid task.

Flags: needinfo?(khushil324)
Comment on attachment 9085132 [details] [diff] [review]
1563000-textbox-html-input.patch

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

::: calendar/base/themes/common/input-fields.css
@@ +4,5 @@
> +
> +@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> +@namespace html url("http://www.w3.org/1999/xhtml");
> +
> +.calendar-input-container {

I'd add it later once needed

@@ +9,5 @@
> +  display: flex;
> +  align-items: stretch;
> +}
> +
> +html|input.calendar-input {

Not sure it's such a good idea to add class="calendar-input" around the place just because it's related to calendar. Say you'd have a place with both calendar and other related items, those should then have the same rules, and the naming assumption kind of breaks down from there... Seems like this should be something more generic? Do we have something already? 
Especially adding flex-grow to such a rule seems bad - that assumes a lot about the surroundings.

@@ +13,5 @@
> +html|input.calendar-input {
> +    flex-grow: 1;
> +    margin: 2px 4px;
> +    padding: 2px 2px 3px;
> +    padding-inline-start: 4px;

mix of 4 space indent and 2 space in this file now. I think even calendar uses 2 for css.

::: mail/themes/windows/mail/accountCreation.css
@@ +67,1 @@
>    font-style: normal;

doesn't seem to belong to this patch perhaps? but is this rule at all needed?
Attachment #9085132 - Flags: review?(mkmelin+mozilla)

(In reply to Magnus Melin [:mkmelin] from comment #6)

+html|input.calendar-input {

Not sure it's such a good idea to add class="calendar-input" around the
place just because it's related to calendar. Say you'd have a place with
both calendar and other related items, those should then have the same
rules, and the naming assumption kind of breaks down from there... Seems
like this should be something more generic? Do we have something already?
Especially adding flex-grow to such a rule seems bad - that assumes a lot
about the surroundings.

I created the input-fields.css in the calendar theme directory because I'm planning to use it across the calendar dialogs and views.
We currently don't have a generic class that applies those styles as those were inherited from the textbox.

Since those styles are pretty much necessary for any inline input field, I can move the input-fields.css into the mail folder, and name those classes something more generic, like:

  • .input-inline
  • .input-color-inline

I removed the flex-grow as it wasn't necessary.

::: mail/themes/windows/mail/accountCreation.css
font-style: normal;

doesn't seem to belong to this patch perhaps? but is this rule at all needed?

It looks odd to me too, but I don't have a Windows machine to test it out.
Let's ask Richard to check it out.

Richard, can you take a look at the font style of the placeholders for those fields that have a .padded class applied to it?
Is that CSS necessary?

Flags: needinfo?(richard.marti)
Flags: needinfo?(mkmelin+mozilla)

.padded is nowhere used in accountWizard.xul. This rule can be removed.

Originally it was used because on Windows 7 the placeholder where in italics and the entered text in normal. This style change could made the textbox width jump and the rule was to set the placeholder to normal all the time to not let jump the textbox width.

Flags: needinfo?(richard.marti)

(In reply to Alessandro Castellani (:aleca) from comment #7)

Since those styles are pretty much necessary for any inline input field, I can move the input-fields.css into the mail folder, and name those classes something more generic, like:

  • .input-inline
  • .input-color-inline

That would be better yes. If it's only about the standard margins/paddings we want to use, we could maybe have the rules apply without class. That is, simply input and input[type="color"] rules.

Flags: needinfo?(mkmelin+mozilla)
Attached patch 1563000-textbox-html-input.patch (obsolete) — Splinter Review

Updated!

If it's only about the standard margins/paddings we want to use, we could maybe have the rules apply without class. That is, simply input and input[type="color"] rules.

I prefer to avoid to do that for now as we might stumble upon some input colors that don't need margin or padding. I prefer to have a dedicated class I can apply if needed.
Once all the textbox are converted, I can have a quick clean up patch to remove unused or redundant classes if necessary.

Attachment #9085132 - Attachment is obsolete: true
Attachment #9085802 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9085802 [details] [diff] [review]
1563000-textbox-html-input.patch

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

LGTM, r=mkmelin with the below

::: calendar/base/content/dialogs/calendar-subscriptions-dialog.xul
@@ -64,5 @@
>        </rows>
>      </grid>
>      <deck id="status-deck" selectedIndex="0">
> -      <hbox class="calendar-subscriptions-status-box">
> -        <image class="calendar-subscriptions-status-icon"/>

i don't think this was supposed to be removed?
Attachment #9085802 - Flags: review?(mkmelin+mozilla) → review+
Attached image Screenshot from 2019-08-16 10-54-08.png (obsolete) —

I removed that because it's not useful, misleading, and a bit ugly.
That's the orange spinner icon, which in that first hbox it stays there, not animated, and not visually useful for the user.
During searches it properly changes tot he animated container.

This is how it looks. It's exactly like this, not moving, not because of the screenshot.
Kind of useless.

Flags: needinfo?(mkmelin+mozilla)

Here's the try-run with the intermittent bct which I can't reproduce locally: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8fbd0fa2d9ccb87c8e4da81129ad5248c45353f9

But it does look like it changes its icon to an animated one while subscribing (look for css hooked up to calendar-subscriptions-status-icon).
I agree this could be done somehow better, but let's leave it for another bug.

Flags: needinfo?(mkmelin+mozilla)
Attachment #9085802 - Attachment is obsolete: true
Attachment #9086112 - Attachment is obsolete: true
Attachment #9086437 - Flags: review+
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a1334c085381
textbox to html input in calendar.base/content/dialogs. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 70.0
You need to log in before you can comment on or make changes to this bug.