Closed Bug 1612776 Opened 1 year ago Closed 1 year ago

Remove usages of `display: -moz-groupbox` and `-moz-appearance: groupbox` (port bug 1590180 to Thunderbird)

Categories

(Thunderbird :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 75.0

People

(Reporter: ntim, Assigned: mkmelin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

See bug 1590180 for how it's done.

XUL groupbox -> HTML fieldset
XUL caption -> HTML legend

Richard/Magnus, can you please look into this ? I'm looking to remove the platform code in bug 1610404.

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

Thanks for filing, I'll take care of this!

Assignee: nobody → mkmelin+mozilla
Flags: needinfo?(richard.marti)
Flags: needinfo?(mkmelin+mozilla)
Summary: Remove usages of `display: -moz-groupbox` → Remove usages of `display: -moz-groupbox` (port bug 1590180 to Thunderbird)

We don't have any usage of display: -moz-groupbox, but we do have -moz-appearance: groupbox

Summary: Remove usages of `display: -moz-groupbox` (port bug 1590180 to Thunderbird) → Remove usages of `display: -moz-groupbox` and `-moz-appearance: groupbox` (port bug 1590180 to Thunderbird)
Blocks: 1612755
No longer blocks: 1610404
Attached patch bug1612776_rm_groupbox.patch (obsolete) — Splinter Review
Attachment #9124347 - Flags: review?(paul)
Attachment #9124347 - Flags: review?(khushil324)
Status: NEW → ASSIGNED
Comment on attachment 9124347 [details] [diff] [review]
bug1612776_rm_groupbox.patch

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

Some minor JS and CSS changes. 

Not related to this but when you open Show Cookies in preference, you see an error in the console: JavaScript error: chrome://messenger/content/preferences/cookies.js, line 533: TypeError: can't access property Symbol.iterator, Services.cookies.enumerator is undefined

::: calendar/base/content/dialogs/calendar-print-dialog.xhtml
@@ +102,5 @@
>            </html:div>
>          </radiogroup>
> +      </html:fieldset>
> +      <html:fieldset id="optionsGroup" label="&calendar.print.optionsGroup.label;">
> +        <hbox class="html:fieldset-title">

Should be using <html:legend>. Not worked well with the script I suppose.

::: calendar/base/content/preferences/views.inc.xhtml
@@ +1,4 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, you can obtain one at http://mozilla.org/MPL/2.0/.
> +    <html:fieldset data-category="paneCalendar">

There is unusual padding between Prefpane heading and this fieldset.

@@ +111,3 @@
>  
> +    <html:fieldset data-category="paneCalendar">
> +        <html:legend>&pref.calendar.view.dayandweekviews.caption;</html:legend>

Alignment issues here.

::: mail/components/accountcreation/content/emailWizard.xhtml
@@ +244,1 @@
>                  class="hub-wrapper config-area"

Indentation is off.

@@ +312,2 @@
>                  class="hub-wrapper config-area"
> +                hidden="hidden"

Indentation is off.

::: mail/components/compose/content/dialogs/EdColorProps.xhtml
@@ +30,5 @@
>  
>    <spacer id="location" offsetY="50" persist="offsetX offsetY"/>
>  
> +  <html:fieldset align="start">
> +    <html:legend>&pageColors.label;</html:legend>

The title of the page is not differentiable. Needs some CSS. Applicable to all the additional dialogs in the compose window.

::: mail/components/compose/content/dialogs/EdInsertChars.xhtml
@@ +24,5 @@
>  
>    <spacer id="location" offsetY="50" persist="offsetX offsetY"/>
>  
> +  <html:fieldset>
> +    <html:legend>&category.label;</html:legend>

Background Color is white. Looks odd.

::: mail/components/compose/content/dialogs/EdInsertMath.xhtml
@@ +35,5 @@
>      <tabs/>
>      <tabpanels oncommand="insertLaTeXCommand(event.target);"/>
>    </tabbox>
>    <spacer class="spacer"/>
> +  <html:fieldset>

Symbols are not loading up. Getting error: 
JavaScript error: chrome://messenger/content/messengercompose/EdInsertMath.js, line 187: SecurityError: The operation is insecure.
This is also the same on Trunk, not due to this patch.

::: mail/components/compose/content/dialogs/EdListProps.xhtml
@@ +44,5 @@
>    <!-- message text and list items are set in JS
>         text value should be identical to string with id=BulletStyle in editor.properties
>    -->
> +  <html:fieldset>
> +    <html:legend id="BulletStyleLabel">&bulletStyle.label;</html:legend>

Here: https://searchfox.org/comm-central/source/mail/components/compose/content/dialogs/EdListProps.js#238, it should be value or textContent.

::: mail/components/compose/content/dialogs/EdTableProps.xhtml
@@ +43,5 @@
>  
>        <!-- TABLE PANEL -->
>        <vbox>
> +        <html:fieldset orient="horizontal">
> +          <html:legend>&size.label;</html:legend>

The background color of the text is white, should be transparent.

::: mail/components/compose/content/dialogs/edImage.inc.xhtml
@@ +153,5 @@
>        </vbox>
>  
>        <hbox id="imageAppearance">
> +        <html:fieldset>
> +          <html:legend id="spacingLabel">&spacingBox.label;</html:legend>

The background color of the text should be transparent.

@@ +212,4 @@
>  
>          <vbox>
> +          <html:fieldset>
> +            <html:legend id="alignLabel">&alignment.label;</html:legend>

Same here.

@@ +237,3 @@
>  
> +          <html:fieldset>
> +            <html:legend id="imagemapLabel">&imagemapBox.label;</html:legend>

Same here.

::: mail/components/im/content/am-im.xhtml
@@ +35,5 @@
>      <hbox class="dialogheader">
>        <label class="dialogheader-title" defaultTitle="&accountWindow.title;"/>
>      </hbox>
>  
> +    <html:fieldset>

We can remove this I suppose.

@@ +44,5 @@
>            <label id="protocolName"/>
>          </vbox>
>        </hbox>
> +    </html:fieldset>
> +    <html:fieldset>

Same here.

::: mail/components/im/content/imAccountWizard.xhtml
@@ +91,1 @@
>          <hbox class="groupbox-title">

Should use html:legend.

::: mail/components/preferences/attachmentReminder.xhtml
@@ +24,5 @@
>    <prefpane id="attachmentReminderOptionsDialogPane">
>      <script src="chrome://messenger/content/preferences/attachmentReminder.js"/>
>      <stringbundle id="bundlePreferences" src="chrome://messenger/locale/preferences/preferences.properties"/>
>  
> +    <html:fieldset>

UI is broke here. We can remove the usage of fieldset.

::: mail/components/preferences/chat.inc.xhtml
@@ +58,3 @@
>  
> +    <html:fieldset data-category="paneChat">
> +      <html:legend data-l10n-id="chat-notifications-title"></html:legend>

The play button should be right-aligned.

::: mail/components/preferences/colors.xhtml
@@ +22,5 @@
>          type="child"
>          dlgbuttons="accept,cancel">
>    <prefpane id="ColorsDialogPane">
>      <hbox>
> +      <html:fieldset>

Not covering 50% width and alignment issues. Color boxes should be right-aligned.

::: mail/components/preferences/fonts.xhtml
@@ +35,5 @@
>      <linkset>
>        <html:link rel="localization" href="messenger/preferences/fonts.ftl"/>
>      </linkset>
>  
> +    <html:fieldset>

Colors button and Plain Text Messages are not aligned(vertically) properly i.e. Two fieldsets are overlapping.

::: mail/components/preferences/privacy.inc.xhtml
@@ +12,5 @@
>        <html:h1 data-l10n-id="privacy-main-header"/>
>      </hbox>
>  
>      <!-- Mail Content -->
> +    <html:fieldset id="mailContentGroup" data-category="panePrivacy">

Buttons are not aligned properly.

::: mail/themes/osx/mail/messenger.css
@@ +737,5 @@
>  .contentTabAddress * {
>    text-shadow: none;
>  }
>  
> +fieldset {

We need to import this file in the external dialog boxes where we want to apply this CSS (in prefpane mainly).

::: mailnews/base/prefs/content/am-archiveoptions.xhtml
@@ +32,5 @@
>                label="&keepFolderStructure.label;"
>                accesskey="&keepFolderStructure.accesskey;"/>
>  
> +    <html:fieldset flex="1">
> +      <html:legend>&archiveExample.label;</html:legend>

UI is broke here.

::: mailnews/base/prefs/content/am-identity-edit.xhtml
@@ +58,5 @@
>  
>      <tabpanels id="identityTabsPanels" flex="1">
>        <!-- Identity Settings Tab -->
>        <vbox flex="1" name="settings">
> +        <html:fieldset>

Buttons are not aligned properly.

::: mailnews/base/prefs/content/am-main.xhtml
@@ +41,5 @@
>      </hbox>
>  
>      <separator class="thin"/>
>  
> +    <html:fieldset style="width: 20em !important;">

Buttons are not aligned properly.
Attachment #9124347 - Flags: review?(khushil324)

(In reply to Khushil Mistry [:khushil324] from comment #5)

Not related to this but when you open Show Cookies in preference, you see an
error in the console: JavaScript error:
chrome://messenger/content/preferences/cookies.js, line 533: TypeError:
can't access property Symbol.iterator, Services.cookies.enumerator is

From bug 1594000 - you have a review waiting now.
The other js error you reported I don't see.

The colors/bg colors issues I don't see on linux. Could you take on fixing whatever styling fixes are needed for mac?

Attached patch bug1612776_rm_groupbox.patch (obsolete) — Splinter Review
Attachment #9124347 - Attachment is obsolete: true
Attachment #9124347 - Flags: review?(paul)
Attachment #9124700 - Flags: review?(khushil324)
Attached patch bug1612776_rm_groupbox.patch (obsolete) — Splinter Review

Everything else was working. r=khushil.

There were some UI issues in views.inc.xhtml and colors.xhtml. I tried to solve those.
Editor dialogs needed some CSS changes.
privacy.inc.xhtml had a typo.

So I made changes in the following files.

calendar/base/content/preferences/views.inc.xhtml
mail/components/preferences/colors.xhtml
mail/components/preferences/privacy.inc.xhtml
mail/themes/shared/mail/EditorDialog.css
mail/themes/shared/mail/incontentprefs/preferences.css

Attachment #9124700 - Attachment is obsolete: true
Attachment #9124700 - Flags: review?(khushil324)
Attachment #9124872 - Flags: review?(mkmelin+mozilla)
Attached image colors.png (obsolete) —
Attached image view-inc.png (obsolete) —

This looks really very weird. Bug 1590180 hasn't changed the preferences files. I think, it's better when we leave them untouched too. And the account manager files too as they get also the in-content styles.

Only change the files which use the default styles like the editor dialogs.

From the comments in the other bugs, <groupbox> will not stay for long so I'd want to get rid of it now. It's correct that to only port the directly "needed" changes it would be less changes.

Incorporating/changing some of Khushil's edits.

For calendar/base/content/preferences/views.inc.xhtml: the layout was messed up on trunk already.
I changed the data now to make sense semantically.

colors.xhtml was just missing the messenger skin import. I think I looked at the wrong file earlier. It's amazing how many color dialogs we have - I'm counting at least 4!!
I did fix the alignments now to look like before (almost, the existing color box far right box alignments were odd so now just slightly to the right)

For indentions when adding a hbox or such, I didn't since it would just make it more difficult to review.
At some point all of these documents will be run through a linter anyway.

Attachment #9124872 - Attachment is obsolete: true
Attachment #9124873 - Attachment is obsolete: true
Attachment #9124874 - Attachment is obsolete: true
Attachment #9124872 - Flags: review?(mkmelin+mozilla)
Attachment #9124992 - Flags: review?(paul)
Comment on attachment 9124992 [details] [diff] [review]
khushilbug1612776_rm_groupbox.patch

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

I looked particularly at the files that Khushil mentioned he had changed (since he had reviewed the rest already).  (A separate patch would have made that easier to do, especially since this is so large already.)  The changes seem fine to me.  

When I checked the calendar preferences page for the views.inc.xhtml changes, overall the new layout looks better than before, but the "hours at a time" text is not aligned with its drop down menu like it was before.  So something to fix here or in a follow-up.

::: calendar/base/content/preferences/views.inc.xhtml
@@ +148,5 @@
> +                            <menuitem label="23" value="23"/>
> +                            <menuitem label="24" value="24"/>
> +                        </menupopup>
> +                    </menulist>
> +                    <label value="&pref.calendar.view.visiblehoursend.label;"/>

This label is the one where the alignment is a bit off.

::: mail/components/preferences/privacy.inc.xhtml
@@ +188,5 @@
>          <spacer flex="1"/>
>          <button id="openJunkLogButton" label="&openJunkLog.label;" accesskey="&openJunkLog.accesskey;"
>                  oncommand="gPrivacyPane.openJunkLog();"/>
>        </hbox>
> +      <hbox align="start" style="padding-bottom:2em;">

Should we be avoiding inline styles?

::: mailnews/base/prefs/content/am-archiveoptions.xhtml
@@ +34,5 @@
>  
> +    <html:fieldset flex="1">
> +      <html:legend>&archiveExample.label;</html:legend>
> +      <hbox flex="1">
> +      <tree id="archiveTree" hidecolumnpicker="true" disabled="true" flex="1" style="min-height:8em;">

Shouldn't we avoid using inline styles?
Attachment #9124992 - Flags: review?(paul) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/b55b6873476a
Remove usages of display: -moz-groupbox and -moz-appearance: groupbox, convert xul:groupbox to html:fieldset (port bug 1590180 to Thunderbird). r=khushil,pmorris

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED

Re inline styles: we should in general avoid them yes. I've used them a few times where it would have been overkill to create a css file for the dialog (and similar). Also since they are often kind of bug fixing for the mixed html/xul layout, which will go away at some point.

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