1344630 - Display parsed value in the corresponding column instead of showing JSON string

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P3)

Description

[Ray Lin[:ralin]]

Right now we display whole JSON string as label & comment, which should be replaced with primary & secondary value in parsed result.

Comment 1

[Ray Lin[:ralin]]

Attachment: Bug 1344630 - Display parsed primary and secondary value in profile item.

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

Comment 2

[Ray Lin[:ralin]]

Comment on attachment 8843860 [details]
Bug 1344630 - Display parsed primary and secondary value in profile item.

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

Comment 3

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8843860 [details]
Bug 1344630 - Display parsed primary and secondary value in profile item.

https://reviewboard.mozilla.org/r/117484/#review119090

getStyleAt provides a type for recognizing which kind of type an item belongs to, and formautofill.xml looks like for "autofill-profile" only. Please add a logic to make sure the item is "autofill-profile" before using formautofill.xml.

::: browser/extensions/formautofill/content/formautofill.xml:68
(Diff revision 1)
>          <![CDATA[
>            let outerBoxRect = this.parentNode.getBoundingClientRect();
> -          let value = this.getAttribute("ac-value");
> +          let {primary, secondary} = JSON.parse(this.getAttribute("ac-value"));
> -          let comment = this.getAttribute("ac-comment");
>  
> -          this._comment.textContent = comment;
> +          this._comment.textContent = primary;

The primary label should be assigned to label, the secondary is for comment.

::: browser/extensions/formautofill/content/formautofill.xml:75
(Diff revision 1)
>  
>            // Make item fit in popup as XUL box could not constrain
>            // item's width
>            this._itemBox.style.width =  outerBoxRect.width + "px";
>            // Use two-lines layout when width is smaller than 150px
>            if (outerBoxRect.width <= 150) {

Since the fonts of AutoComplete Popup is different with the focused input, we have to file a new bug to refine the condition of two-line case.

Comment 4

[Ray Lin[:ralin]]

Comment on attachment 8843860 [details]
Bug 1344630 - Display parsed primary and secondary value in profile item.

https://reviewboard.mozilla.org/r/117484/#review119090

Thanks for reviewing.

As this stylesheet is only imported by our extension, so we should be able to ensure "formautofill.xml" is used properly.
https://dxr.mozilla.org/mozilla-central/source/browser/extensions/formautofill/content/formautofill.css#5-13

> The primary label should be assigned to label, the secondary is for comment.

ah! My fault didn't find this careless mistake... 
fixed in new revision.

> Since the fonts of AutoComplete Popup is different with the focused input, we have to file a new bug to refine the condition of two-line case.

yeah, we should start test on different coner cases to find out how to optimize the condition (in a spun off bug).

Comment 5

[Ray Lin[:ralin]]

Comment on attachment 8843860 [details]
Bug 1344630 - Display parsed primary and secondary value in profile item.

Sorry to add back review flag in this way, since review(for revision 1) cancel just happened right after I pushed revision 2.

Comment 6

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8843860 [details]
Bug 1344630 - Display parsed primary and secondary value in profile item.

https://reviewboard.mozilla.org/r/117484/#review119130

Looks good to me!

Comment 7

[Carsten Book [:Tomcat]]

seems this still need a final review/ship-it in mozreview before we can use the autoland feature. Can you fix this in mozreview? thanks

Comment 8

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8843860 [details]
Bug 1344630 - Display parsed primary and secondary value in profile item.

https://reviewboard.mozilla.org/r/117484/#review119438

Comment 9

[Sean Lee [:seanlee][:weilonge]]

Hey Tomcat,

I think I already did a final review at
https://reviewboard.mozilla.org/r/117484/#review119130

However, I gave it a r+ in mozreview again.
https://reviewboard.mozilla.org/r/117484/#review119438

Could you check if I did it correctly?

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Do you have Level 3 commit access? Because that's required for Autoland. If you do and it's still not working, you may need to manually associate your LDAP account with MozReview:
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html#manually-associating-your-ldap-account-with-mozreview

Comment 11

[Sean Lee [:seanlee][:weilonge]]

Hey Ryan, Thanks. I don't have lv3 yet.

Hey MattN, Could you help to land this bug? thanks.

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8843860 [details]
Bug 1344630 - Display parsed primary and secondary value in profile item.

https://reviewboard.mozilla.org/r/117484/#review119904

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ecb6fcbdb2a
Display parsed primary and secondary value in profile item. r=MattN,seanlee

Comment 14

[Matthew N. [:MattN]]

Done

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1ecb6fcbdb2a

Comment 16

[Adrian Florinescu [:aflorinescu]]

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