Closed
Bug 1557216
Opened 5 years ago
Closed 5 years ago
remove grid usage from comm/mailnews/addrbook/prefs/content/pref-directory-add.xul
Categories
(Thunderbird :: Address Book, task)
Thunderbird
Address Book
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 70.0
People
(Reporter: khushil324, Assigned: khushil324)
References
Details
Attachments
(4 files, 8 obsolete files)
Screenshot 2019-06-06 at 11.01.39 AM.png
Run window.openDialog("chrome://messenger/content/addressbook/pref-directory-add.xul", "", "chrome,modal,resizable=no,centerscreen", null) to open the dialogue box.
|
||
Comment 1•5 years ago
|
||
Available from Preferences | Composition | Edit Directories... | Add
Blocks: tb-burn-xul-grids
|
Assignee | |
Updated•5 years ago
|
Assignee: nobody → khushil324
|
|
Assignee | |
Comment 2•5 years ago
|
||
Attachment #9079806 -
Flags: review?(mkmelin+mozilla)
|
|
Assignee | |
Updated•5 years ago
|
Status: NEW → ASSIGNED
|
|
||
Comment 3•5 years ago
|
||
Comment on attachment 9079806 [details] [diff] [review]
Bug-1557216_remove-grid-pref-directory-add.patch
Review of attachment 9079806 [details] [diff] [review]:
-----------------------------------------------------------------
::: mailnews/addrbook/prefs/content/pref-directory-add.xul
@@ +34,5 @@
>
> <tabpanels id="directoryTabPanels" flex="1">
> <vbox>
> + <div xmlns="http://www.w3.org/1999/xhtml" style="display:inline-grid;
> + grid-template-columns:auto auto auto;">
I think we need to figure out classes for the inline styles instead.
@@ +72,5 @@
> + <xul:label value="&portNumber.label;"
> + accesskey="&portNumber.accesskey;"
> + control="port"/>
> + </div>
> + <div style="margin: 5px;">
things like this is really wrong.
in this case I believe it's because of the textbox vs input, but those textboxes will all be inputs soon and then it all aligns like it should
@@ +120,5 @@
> + type="number"
> + class="size5"
> + min="1"
> + max="2147483647"
> + value="100"/>
please keep the formatting kind of like it was (not everything one attr per row)
Attachment #9079806 -
Flags: review?(mkmelin+mozilla)
|
|
Assignee | |
Comment 4•5 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #3)
I think we need to figure out classes for the inline styles instead.
Should I create the util CSS file for grid usage and import it wherever we are using?
|
|
Assignee | |
Comment 5•5 years ago
|
||
Attachment #9079806 -
Attachment is obsolete: true
Attachment #9080582 -
Flags: review?(mkmelin+mozilla)
|
|
||
Comment 6•5 years ago
|
||
Comment on attachment 9080582 [details] [diff] [review]
Bug-1557216_remove-grid-pref-directory-add.patch
Review of attachment 9080582 [details] [diff] [review]:
-----------------------------------------------------------------
Maybe Aleca has some thoughts on this
::: mail/themes/shared/mail/gridLayout.css
@@ +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/. */
> +.threeColumnGrid {
better with namings using hypens, like three-column-grid
@@ +10,5 @@
> + display:inline-grid;
> + grid-template-columns: auto auto;
> +}
> +
> +table.threeColumnGrid textbox, menulist {
not sure why menulist is in here
@@ +18,5 @@
> +table.twoColumnGrid textbox, menulist {
> + width: 100%;
> +}
> +
> +.divAlignCenter {
and no div in the name please
Attachment #9080582 -
Flags: feedback?(alessandro)
|
||
Comment 7•5 years ago
|
||
Comment on attachment 9080582 [details] [diff] [review]
Bug-1557216_remove-grid-pref-directory-add.patch
Review of attachment 9080582 [details] [diff] [review]:
-----------------------------------------------------------------
I'm not sure about this approach.
What's the reason behind going with divs instead of hbox and vbox?
I'm not against having a generic gridLayout file to use when necessary, but I think we should first decide which approach we want to follow between flexbox and CSS grid.
::: mail/themes/shared/mail/gridLayout.css
@@ +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/. */
> +.threeColumnGrid {
If we're going with this approach, I think we should rename the classes to list the main property first and then the modifier.
eg. grid-three-column.
@@ +10,5 @@
> + display:inline-grid;
> + grid-template-columns: auto auto;
> +}
> +
> +table.threeColumnGrid textbox, menulist {
I don't seem to be able to find the table element used here, why the class applied to it?
Also, don't style a common element like menulist without a proper selector or parent-child relationship, otherwise all the menulist elements will be affected if we include this file.
@@ +14,5 @@
> +table.threeColumnGrid textbox, menulist {
> + width: 100%;
> +}
> +
> +table.twoColumnGrid textbox, menulist {
Why the repetition?
Merge this declaration with the one above.
@@ +18,5 @@
> +table.twoColumnGrid textbox, menulist {
> + width: 100%;
> +}
> +
> +.divAlignCenter {
This should also be .flex-items-center
::: mailnews/addrbook/prefs/content/pref-directory-add.xul
@@ +35,4 @@
>
> <tabpanels id="directoryTabPanels" flex="1">
> <vbox>
> + <div xmlns="http://www.w3.org/1999/xhtml" class="threeColumnGrid">
Any particular reason why we're implementing divs instead of hbox and vbox?
@@ +144,5 @@
> + <xul:label value="&saslMechanism.label;" control="saslMechanism"
> + accesskey="&saslMechanism.accesskey;"/>
> + </div>
> + <div>
> + <xul:menulist id="saslMechanism" style="width:100%;">
Do you need the inline style if you're applying the width:100% attribute in the CSS file?
Attachment #9080582 -
Flags: feedback?(alessandro) → feedback-
|
|
||
Comment 8•5 years ago
|
||
Some alignment issues on Linux
|
|
Assignee | |
Comment 9•5 years ago
|
||
So currently we have three approaches:
- hbox and vbox
- html:div
- html:table
If the layout is very simple and doesn't have multiple elements i.e. we just have a column with the same elements, I use hbox and vbox. If suppose column has textbox and menlist in it, it will have alignment issues in terms of rows.
If the layout is small but has columns with different elements like textbox, button, menulist, etc. I try to use html:div. But width property is not so adjustable here. So we will have some width related issue like all labels should occupy width same as a label with a maximum width. But here, labels generally occupy width up to some percentage of the full width like 30% which is generally more than the label with the maximum width. In such cases, I use html:tabel.
|
|
Assignee | |
Comment 10•5 years ago
|
||
(In reply to Alessandro Castellani (:aleca) from comment #8)
Some alignment issues on Linux
I find this alignment issue on the trunk also and it is due to our usage of html:input. Currently, I will solve it with inline styling. We can take care of this thing when we convert all textbox to html:input.
|
|
Assignee | |
Comment 11•5 years ago
|
||
Attachment #9080582 -
Attachment is obsolete: true
Attachment #9080582 -
Flags: review?(mkmelin+mozilla)
Attachment #9080713 -
Flags: review?(alessandro)
|
|
||
Comment 12•5 years ago
|
||
(In reply to Khushil Mistry [:khushil324] from comment #9)
So currently we have three approaches:
3) html:table
Should only use table for cases where the data is semantically tabular. It should not be used for layout.
|
|
||
Comment 13•5 years ago
|
||
Comment on attachment 9080713 [details] [diff] [review]
Bug-1557216_remove-grid-pref-directory-add.patch
Review of attachment 9080713 [details] [diff] [review]:
-----------------------------------------------------------------
::: mail/themes/shared/jar.inc.mn
@@ +122,4 @@
> skin/classic/messenger/tagColors.css (../shared/mail/tagColors.css)
> skin/classic/messenger/shared/smime/smime-compose.css (../shared/mail/smime/smime-compose.css)
> skin/classic/messenger/shared/customizableui/panelUI.css (../shared/customizableui/panelUI.css)
> + skin/classic/messenger/shared/gridLayout.css (../shared/mail/gridLayout.css)
It would be nicer to name new things all lower case, and use hypen instead of camelCasing.
::: mail/themes/shared/mail/gridLayout.css
@@ +11,5 @@
> + grid-template-columns: auto auto;
> +}
> +
> +.grid-three-column textbox, menulist, input,
> +.grid-two-column textbox, menulist, input {
the menulist and input should not be there.
(Did you mean when they are children of **-two-column? That's not what this selector does, it applies to all menulist and input elements)
|
|
||
Comment 14•5 years ago
|
||
Comment on attachment 9080713 [details] [diff] [review]
Bug-1557216_remove-grid-pref-directory-add.patch
Review of attachment 9080713 [details] [diff] [review]:
-----------------------------------------------------------------
Everything that Magnus said, plus this inline style.
I still think this should be achieved with hbox and vbox.
This is a fairly simple grid with a few elements that don't need this extra styling and complexity.
Magnus, what do you think?
::: mailnews/addrbook/prefs/content/pref-directory-add.xul
@@ +72,5 @@
> + <xul:label value="&portNumber.label;"
> + accesskey="&portNumber.accesskey;"
> + control="port"/>
> + </div>
> + <div style="margin:5px">
As magnus said, this is wrong and you shouldn't use inline styling for pixel spacing.
Attachment #9080713 -
Flags: review?(alessandro) → review-
|
|
||
Comment 15•5 years ago
|
||
the 5px comes from the input/textbox different styling - I'd leave that "bug" in and let the remaining textbox conversion take care (automatcially).
I'm not sure if hbox/vbox or div is better. On one hand, html conversion will happen likely before next ESR, but that there is things like <xul:spacer flex="1"/> in there makes it kind of just a hack...
|
|
Assignee | |
Comment 16•5 years ago
|
||
Attachment #9080713 -
Attachment is obsolete: true
Attachment #9080966 -
Flags: review?(mkmelin+mozilla)
|
|
||
Comment 17•5 years ago
|
||
Comment on attachment 9080966 [details] [diff] [review]
Bug-1557216_remove-grid-pref-directory-add.patch
Review of attachment 9080966 [details] [diff] [review]:
-----------------------------------------------------------------
grid-layout.css is not included in this patch
::: mailnews/addrbook/prefs/content/pref-directory-add.xul
@@ +43,5 @@
> + <div>
> + <xul:textbox id="description"/>
> + </div>
> + <div>
> + <xul:spacer flex="1"/>
Hmm, now in the grid layout, is this at all needed? If anything, I'd expect perhaps an &nsbp; in the div would do. Does the flex work in here?
Attachment #9080966 -
Flags: review?(mkmelin+mozilla) → review-
|
|
Assignee | |
Comment 18•5 years ago
|
||
I have added empty <div/> and it worked.
Attachment #9080966 -
Attachment is obsolete: true
Attachment #9081604 -
Flags: review?(mkmelin+mozilla)
|
|
||
Comment 19•5 years ago
|
||
Comment on attachment 9081604 [details] [diff] [review]
Bug-1557216_remove-grid-pref-directory-add.patch
Review of attachment 9081604 [details] [diff] [review]:
-----------------------------------------------------------------
::: mail/themes/shared/mail/grid-layout.css
@@ +1,3 @@
> +/* 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/. */
please add a blank line after the license block
::: mailnews/addrbook/prefs/content/pref-directory-add.xul
@@ +62,5 @@
> + </div>
> + <div>
> + <xul:button label="&findButton.label;"
> + accesskey="&findButton.accesskey;" disabled="true"/>
> + </div>
this button is not vertically centred anymore
@@ +73,5 @@
> + <input id="port" type="number" class="size5"
> + min="1" max="65535"
> + disableiflocked="true"/>
> + </div>
> + <div/>
div is not a self closing tag in html, so please write it out as <div></div>
(It will work now as xhtml but better make it non-closing for maximum forward compatibility, e.g. it could break as html.)
Attachment #9081604 -
Flags: review?(mkmelin+mozilla)
Attachment #9081604 -
Flags: review-
Attachment #9081604 -
Flags: feedback+
|
|
Assignee | |
Comment 20•5 years ago
|
||
Can you (In reply to Magnus Melin [:mkmelin] from comment #19)
this button is not vertically centred anymore
Can you attach a SS for this as I am not able to see this issue on Mac.
|
|
||
Comment 21•5 years ago
|
||
Screenshot
|
|
Assignee | |
Comment 22•5 years ago
|
||
Were you able to see this problem when the usage of <spacer> was there?
|
|
||
Comment 23•5 years ago
|
||
Yes it is the same
|
|
Assignee | |
Comment 24•5 years ago
|
||
Attachment #9081604 -
Attachment is obsolete: true
Attachment #9081958 -
Flags: review?(mkmelin+mozilla)
|
|
||
Comment 25•5 years ago
|
||
Comment on attachment 9081958 [details] [diff] [review]
Bug-1557216_remove-grid-pref-directory-add.patch
Review of attachment 9081958 [details] [diff] [review]:
-----------------------------------------------------------------
The find button is now vertically centered, but it is crammed against the input filed (no padding, margin or whatever)
Attachment #9081958 -
Flags: review?(mkmelin+mozilla) → review-
|
|
Assignee | |
Comment 26•5 years ago
|
||
Verified on Linux Machine.
Attachment #9081958 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•5 years ago
|
Attachment #9082737 -
Flags: review?(mkmelin+mozilla)
|
|
Assignee | |
Updated•5 years ago
|
Attachment #9082737 -
Flags: review?(mkmelin+mozilla)
|
|
Assignee | |
Comment 27•5 years ago
|
||
Attachment #9082737 -
Attachment is obsolete: true
Attachment #9082749 -
Flags: review?(mkmelin+mozilla)
|
|
||
Comment 28•5 years ago
|
||
Comment on attachment 9082749 [details] [diff] [review]
Bug-1557216_remove-grid-pref-directory-add.patch
Review of attachment 9082749 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good now, just some nits. r=mkmelin
::: mail/themes/shared/mail/grid-layout.css
@@ +6,5 @@
> + display:inline-grid;
> + grid-template-columns: auto auto auto;
> +}
> +
> +.grid-two-column {
move befor grid-three-column
@@ +16,5 @@
> +.grid-three-column menulist,
> +.grid-three-column textarea,
> +.grid-two-column textbox,
> +.grid-two-column menulist,
> +.grid-two-column textarea {
could you put these in numerical order, two before three like
.grid-two-column textbox,
.grid-two-column menulist,
.grid-two-column textarea,
.grid-three-column textbox,
.grid-three-column menulist,
.grid-three-column textarea
::: mailnews/addrbook/prefs/content/pref-directory-add.xul
@@ +59,5 @@
> + </div>
> + <div>
> + <xul:textbox id="basedn" disableiflocked="true" class="uri-element"/>
> + </div>
> + <div class="flex-items-center" style="justify-content:center;">
please instead of inlining the style, add a .flex-content-center rule
Attachment #9082749 -
Flags: review?(mkmelin+mozilla) → review+
|
|
Assignee | |
Comment 29•5 years ago
|
||
Attachment #9082749 -
Attachment is obsolete: true
Attachment #9083001 -
Flags: review+
|
|
Assignee | |
Updated•5 years ago
|
Keywords: checkin-needed
|
||
Comment 30•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0e4007423df0
remove grid usage from pref-directory-add.xul. r=mkmelin
|
||
Updated•5 years ago
|
Target Milestone: --- → Thunderbird 70.0
You need to log in
before you can comment on or make changes to this bug.
Description
•