1326139 - Implement two column layout for profile item binding

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Ray Lin[:ralin]]

Implement two column layout based on profile item binding prototype. Also, we need to ensure that data and its comment are correctly filled in popup table.

Comment 1

[Ray Lin[:ralin]]

Attachment: Bug 1326139 - Implement profile item two column layout.

Review commit: https://reviewboard.mozilla.org/r/109488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109488/

Comment 2

[Ray Lin[:ralin]]

Comment on attachment 8833251 [details]
Bug 1326139 - Implement profile item two column layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109488/diff/1-2/

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8833251 [details]
Bug 1326139 - Implement profile item two column layout.

https://reviewboard.mozilla.org/r/109488/#review113090

::: browser/extensions/formautofill/content/formautofill.css:28
(Diff revision 2)
> +  background-color: #F2F2F2;
> +}

Colors need to go in a skin stylesheet like we discussed before. This stylesheet should only be about layout things that aren't a matter of style.

You will probably want to create a shared skin stylesheet and later we may have platform-specific styles.

::: browser/extensions/formautofill/content/formautofill.css:62
(Diff revision 2)
> +  font-size: 13px;
> +}
> +
> +.profile-item-box > .profile-item-col > .profile-comment {
> +  font-size: 10px;

Fonts normally should use `em` units since the user can override the fonts at the OS level and this should respect that.

::: browser/extensions/formautofill/content/formautofill.css:73
(Diff revision 2)
> +
> +.profile-item-box[size="small"] {
> +  padding-top: 10px;
> +  padding-bottom: 10px;
> +  height: auto;
> +  min-height: 30px;

What's this for? Does it need to vary by font-size?

::: toolkit/components/satchel/AutoCompletePopup.jsm:155
(Diff revision 2)
>          tabbrowser.selectedBrowser != browser) {
>        // We were sent a message from a window or tab that went into the
>        // background, so we'll ignore it for now.
>        return;
>      }
> +    let firstResultStyle = results[0].style;

Nit: You could inline this with the setAttribute call.

::: toolkit/components/satchel/AutoCompletePopup.jsm:159
(Diff revision 2)
>      this.openedPopup.hidden = false;
> +    // the layout varies according to different result type
> +    this.openedPopup.setAttribute("first-result-style", firstResultStyle);

Nit: Move the setAttribute before .hidden so that the class is set before the element starts getting styling calculated on it.

::: toolkit/components/satchel/AutoCompletePopup.jsm:161
(Diff revision 2)
>  
>      this.weakBrowser = Cu.getWeakReference(browser);
>      this.openedPopup = browser.autoCompletePopup;
>      this.openedPopup.hidden = false;
> +    // the layout varies according to different result type
> +    this.openedPopup.setAttribute("first-result-style", firstResultStyle);

Nit: XUL attributes are normally all lowercase with no hyphens.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8833251 [details]
Bug 1326139 - Implement profile item two column layout.

https://reviewboard.mozilla.org/r/109488/#review113090

> Colors need to go in a skin stylesheet like we discussed before. This stylesheet should only be about layout things that aren't a matter of style.
> 
> You will probably want to create a shared skin stylesheet and later we may have platform-specific styles.

I think it would be best to have a separate "autocomplete-item.css" file for the extension which is scoped to the richlistitem which ensures that our selectors won't interfere with non-autofill results which makes it less fragile. See an example at https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/toolkit/content/widgets/autocomplete.xml#703

Basically add ```xml
<stylesheet src="chrome://formautofill/skin/autocomplete-item.css"/>
``` to our binding.

Comment 5

[Ray Lin[:ralin]]

Comment on attachment 8833251 [details]
Bug 1326139 - Implement profile item two column layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109488/diff/2-3/

Comment 6

[Ray Lin[:ralin]]

Comment on attachment 8833251 [details]
Bug 1326139 - Implement profile item two column layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109488/diff/3-4/

Comment 7

[Ray Lin[:ralin]]

Comment on attachment 8833251 [details]
Bug 1326139 - Implement profile item two column layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109488/diff/4-5/

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8833251 [details]
Bug 1326139 - Implement profile item two column layout.

https://reviewboard.mozilla.org/r/109488/#review115810

Hey Ray, looks pretty good. Just a few comments.

::: browser/extensions/formautofill/content/formautofill.xml:72
(Diff revision 5)
>            let comment = this.getAttribute("ac-comment");
>  
>            this._comment.textContent = comment;
>            this._label.textContent = value;
>  
> +          this._itemBox.style.width =  outerBoxRect.width + "px";

Could you explain what this is for? Perhaps in a comment?

::: browser/extensions/formautofill/jar.mn:12
(Diff revision 5)
> +% skin formautofill classic/1.0 %skin/linux/
> +% skin formautofill classic/1.0 %skin/osx/ os=Darwin

Hmm… I guess we don't need to specify an os=… here?

::: browser/extensions/formautofill/jar.mn:15
(Diff revision 5)
> +% skin formautofill-shared classic/1.0 %skin/shared/
> +  skin/  (skin/*)

I think we could have avoided the extra package name by adding more copying rules (at the expense of extra copies of the shared file) but I guess this is fine for now since it's what others are doing.

::: browser/extensions/formautofill/skin/shared/autocomplete-item.css:13
(Diff revision 5)
> +.profile-item-box {
> +  box-sizing: border-box;
> +  border-bottom: 1px solid rgba(38,38,38,.15);
> +  padding-inline-start: 16px;
> +  padding-inline-end: 10px;
> +  height: 30px;

We normally don't fix the height when there's text inside as the height should grow as the text grows. Can you use top and bottom padding or margin instead?

Comment 9

[Ray Lin[:ralin]]

Comment on attachment 8833251 [details]
Bug 1326139 - Implement profile item two column layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109488/diff/5-6/

Comment 10

[Ray Lin[:ralin]]

Comment on attachment 8833251 [details]
Bug 1326139 - Implement profile item two column layout.

https://reviewboard.mozilla.org/r/109488/#review115810

Thanks you Matt, you helped me a lot in workweek.

> Could you explain what this is for? Perhaps in a comment?

This is becuase HTML flexbox could not fit in/expand correctly inside XUL box. I'll add a comment.

> Hmm… I guess we don't need to specify an os=… here?

It seems we still need "os=" if we need platform-specific style.
I've tried to remove them and rebuilt, but it turned out that these rule are overrided by the last one:
`% skin formautofill classic/1.0 %skin/windows/` and all platform use window's style.

> We normally don't fix the height when there's text inside as the height should grow as the text grows. Can you use top and bottom padding or margin instead?

the text-height is around 17px. I added top/bottom padding: 6px, and it should be close enough to spec.

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8833251 [details]
Bug 1326139 - Implement profile item two column layout.

https://reviewboard.mozilla.org/r/109488/#review115810

> It seems we still need "os=" if we need platform-specific style.
> I've tried to remove them and rebuilt, but it turned out that these rule are overrided by the last one:
> `% skin formautofill classic/1.0 %skin/windows/` and all platform use window's style.

Yeah, I meant that there wasn't os… for linux in the patch and wasn't sure how/if that worked. I guess you're saying the last matching one wins (which is what I figured) so that's fine.

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8833251 [details]
Bug 1326139 - Implement profile item two column layout.

https://reviewboard.mozilla.org/r/109488/#review116308

Thanks

Comment 13

[Ray Lin[:ralin]]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #12)
> Comment on attachment 8833251 [details]
> Bug 1326139 - Implement profile item two column layout.
> 
> https://reviewboard.mozilla.org/r/109488/#review116308
> 
> Thanks

Thank you :)

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40b53192a7d12c1dd3b9673ca0cdb4df945bc8e9
will checkin-needed if the try result is good.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17c179163218
Implement profile item two column layout. r=MattN

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/17c179163218

Comment 16

[Adrian Florinescu [:aflorinescu]]

As per San-Francisco meeting with :vchen, marking this bug as qe-.