The default bug view has changed. See this FAQ.

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
20 days ago
18 days 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

20 days 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

20 days 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

20 days 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

20 days 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

20 days 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

20 days 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

20 days 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

18 days 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

18 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1ecb6fcbdb2a
Status: ASSIGNED → RESOLVED
Last Resolved: 18 days 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.