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

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Form Manager
P3
normal
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: ralin, Assigned: ralin)

Tracking

53 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [form autofill:M1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
Right now we display whole JSON string as label & comment, which should be replaced with primary & secondary value in parsed result.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

4 months ago
mozreview-review
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.
Attachment #8843860 - Flags: review?(selee)
(Assignee)

Comment 4

4 months ago
mozreview-review-reply
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).
(Assignee)

Comment 5

4 months ago
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.
Attachment #8843860 - Flags: review?(selee)

Comment 6

4 months ago
mozreview-review
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!
Attachment #8843860 - Flags: review?(selee) → review+
(Assignee)

Updated

4 months ago
Keywords: checkin-needed
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
Flags: needinfo?(selee)
Keywords: checkin-needed

Comment 8

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

https://reviewboard.mozilla.org/r/117484/#review119438
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?
Flags: needinfo?(selee)
Flags: needinfo?(cbook)
Keywords: checkin-needed
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
Flags: needinfo?(cbook)
Keywords: checkin-needed
Hey Ryan, Thanks. I don't have lv3 yet.

Hey MattN, Could you help to land this bug? thanks.
Flags: needinfo?(MattN+bmo)
Attachment #8843860 - Flags: review?(MattN+bmo)
Comment on attachment 8843860 [details]
Bug 1344630 - Display parsed primary and secondary value in profile item.

https://reviewboard.mozilla.org/r/117484/#review119904
Attachment #8843860 - Flags: review?(MattN+bmo) → review+

Comment 13

4 months ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/1ecb6fcbdb2a
Display parsed primary and secondary value in profile item. r=MattN,seanlee
Done
Flags: needinfo?(MattN+bmo)

Comment 15

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1ecb6fcbdb2a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.