Closed Bug 1300996 Opened 8 years ago Closed 7 years ago

Show a preview of what would be filled when a form autofill autocomplete result is highlighted

Categories

(Toolkit :: Form Manager, enhancement, P3)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: MattN, Assigned: ralin)

References

(Depends on 2 open bugs, Blocks 3 open bugs)

Details

(Whiteboard: [form autofill:M3] ETA:612)

Attachments

(4 files)

Bug 1300996 - Part 1: Set selectedIndex of autocomplete popup when mousemove over profile item.
59 bytes, text/x-review-board-request
adw
: review+
Details
Bug 1300996 - Part 2: Show preview text on and highlight the fields that would be filled.
59 bytes, text/x-review-board-request
MattN
: review+
lchang
: review+
Details
Bug 1300996 - Part 3: Handle the highlight style if the filled fields being changed by user.
59 bytes, text/x-review-board-request
lchang
: review+
MattN
: review+
Details
Bug 1300996 - Part 4: Add mochitest for form autofill preview and highlight.
59 bytes, text/x-review-board-request
MattN
: review+
lchang
: review+
Details 
Highlight the fields that would be filled if the user selected the autocomplete entry. The content process can use the API provided by bug 1300990 to do the mapping of profile data to fields.
Blocks: 990192
Depends on: 1301003
Whiteboard: [form autofill] → [form autofill:MVP]
Depends on: 1340483
Depends on: 1340468
Since preview API for <input> and highlight pseudo-class are both landed, I'd like to integrate them into fully functional preview feature here.
Assignee: nobody → ralin
Status: NEW → ASSIGNED
Depends on: 1359355
Whiteboard: [form autofill:MVP] → [form autofill:M3]
Depends on: 1361244
Comment on attachment 8861335 [details]
Bug 1300996 - Part 1: Set selectedIndex of autocomplete popup when mousemove over profile item.

https://reviewboard.mozilla.org/r/133322/#review139536

::: commit-message-649d8:6
(Diff revision 2)
> +Bug 1300996 - Set selectedIndex of autocomplete popup when mousemove over profile item. r=adw
> +
> +Currently mouseover autocomplete item would not change the selectedIndex
> +of popup but show grey highlight. For form autofill, we don't want
> +two indicators to represent the selection of keyboard and mouse
> +separately that mihgt confuse user which profile preview is exactly

Nit: typo: might

::: browser/extensions/formautofill/content/formautofill.xml:29
(Diff revision 2)
>      <implementation implements="nsIDOMXULSelectControlItemElement">
> +      <field name="selectedByMousedOver">true</field>

Perhaps this should also be defined as `false` in an ancestor/base binding (where appropriate)? I'm not sure if that's that's the convention here but we normally like to define members on JS prototypes before they are set and this feels similar. I would also add a comment there describing what it's for.

::: browser/extensions/formautofill/content/formautofill.xml:30
(Diff revision 2)
>          </div>
>        </div>
>      </xbl:content>
>  
>      <implementation implements="nsIDOMXULSelectControlItemElement">
> +      <field name="selectedByMousedOver">true</field>

Nit: I think `selectedByMouseOver` (no "d") is a bit clearer.

::: toolkit/content/widgets/autocomplete.xml:2550
(Diff revision 2)
> +        if (item.selectedByMousedOver) {
> +          this.selectedIndex = index;

Could you add an autocomplete test for this? Not a formautofill-specific one?
Comment on attachment 8861336 [details]
Bug 1300996 - Part 2: Show preview text on and highlight the fields that would be filled.

https://reviewboard.mozilla.org/r/133320/#review139544

Thanks! A mochitest will be really useful for this.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:155
(Diff revision 2)
>      for (let fieldDetail of this.fieldDetails) {
> +      let element = fieldDetail.elementWeakRef.get();
>        let value = profile[fieldDetail.fieldName] || "";
>  
>        // Skip the fields that already has text entered
> -      if (fieldDetail.element.value) {
> +      if (element.value) {

element can be `null`

::: browser/extensions/formautofill/FormAutofillHandler.jsm:170
(Diff revision 2)
> +   * Clear preview text and background highlight of all fields.
> +   *
> +   * @param {Boolean} force
> +   *        Used to force remove highlight.
> +   */
> +  clearPreviewedFormFields(force = false) {

So are you wanting to land this without handling a user correcting a filled field?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:196
(Diff revision 2)
> +    let element = fieldDetail.elementWeakRef.get();
> +

What happens if `element` is null because the element was deleted? Will it cause a crash? I don't think we should ignore that case by just doing an early return  as it may be useful for debugging so maybe a log.warn would be useful?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:199
(Diff revision 2)
> -    */
> +   */
> +  updateFieldHighlightVisibility(fieldDetail, show) {
> +    let element = fieldDetail.elementWeakRef.get();
> +
> +    if (show) {
> +      this.winUtils.addManuallyManagedState(element, "-moz-autofill");

Please add an autofill mochitest-plaintest for this.
Attachment #8861336 - Flags: review?(MattN+bmo)
Comment on attachment 8861335 [details]
Bug 1300996 - Part 1: Set selectedIndex of autocomplete popup when mousemove over profile item.

https://reviewboard.mozilla.org/r/133322/#review141366

r+ with Matt's comments addressed (thanks Matt)
Attachment #8861335 - Flags: review?(adw) → review+
Comment on attachment 8861335 [details]
Bug 1300996 - Part 1: Set selectedIndex of autocomplete popup when mousemove over profile item.

https://reviewboard.mozilla.org/r/133322/#review141366

Thank you Matt, Drew

I've updated this patch, could you help to double check the test case if you get time?
Comment on attachment 8861336 [details]
Bug 1300996 - Part 2: Show preview text on and highlight the fields that would be filled.

https://reviewboard.mozilla.org/r/133320/#review139544

Thank for reviewing :)

> So are you wanting to land this without handling a user correcting a filled field?

I filed bug 1359355 for that, but it's fine to handle in this bug if you prefer to.

> What happens if `element` is null because the element was deleted? Will it cause a crash? I don't think we should ignore that case by just doing an early return  as it may be useful for debugging so maybe a log.warn would be useful?

yeah, it probably crash if element is null. I'll add an early return and log.warn below. Also, I wonder if we could do more in this issue? Like wrapping fieldDetail.elementWeakRef.get(); as a getter function to log.warn for all callers instead of just this one

> Please add an autofill mochitest-plaintest for this.

Currently there's no way to know which "ManuallyManagedState" has been added to the element, so could we check filter style instead? 
Another question would be: mochitest-plaintest could not access profile item and its style(perhaps indirectly through parent_utils?), would browser chrome test be a better choice for this case?
Rebased the patch, and fixed failed test(eslint + mochitest).
Comment on attachment 8861336 [details]
Bug 1300996 - Part 2: Show preview text on and highlight the fields that would be filled.

https://reviewboard.mozilla.org/r/133320/#review145874

::: browser/extensions/formautofill/FormAutofillHandler.jsm:157
(Diff revision 8)
>      log.debug("clear previewed fields in:", this.form);
> -    /*
> +
>      for (let fieldDetail of this.fieldDetails) {
> -      // TODO: Clear preview text
> +      let element = fieldDetail.elementWeakRef.get();
>  
> -      // We keep the highlight of all fields if this form has
> +      element.previewValue = "";

Needs to check if the element exists.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:181
(Diff revision 8)
> -    */
> +   */
> +  transitionFieldState(fieldDetail, nextState) {
> +    let element = fieldDetail.elementWeakRef.get();
> +
> +    if (!element || !(nextState in this.fieldStateEnum)) {
> +      log.warn(fieldDetail.fieldName, "is unreachable while state transition");

This message doesn't cover the second condition. You might need to separate them.
Attachment #8861336 - Flags: review?(lchang) → review+
Comment on attachment 8861336 [details]
Bug 1300996 - Part 2: Show preview text on and highlight the fields that would be filled.

https://reviewboard.mozilla.org/r/133320/#review145928

::: browser/extensions/formautofill/FormAutofillHandler.jsm:177
(Diff revision 8)
> +   * @param {Object} fieldDetail
> +   *        A fieldDetail of which its element is about to update the state.
> +   * @param {string} nextState
> +   *        Used to determine the next state to transition.
> -    */
> +   */
> +  transitionFieldState(fieldDetail, nextState) {

nit: It's a bit weird to me to use "transition" here and in the comment. How about `changeFieldState`?
Comment on attachment 8868955 [details]
Bug 1300996 - Part 3: Handle the highlight style if the filled fields being changed by user.

https://reviewboard.mozilla.org/r/140624/#review145922

::: browser/extensions/formautofill/FormAutofillHandler.jsm:131
(Diff revision 4)
> +    // Since SetUserInput asynchronously dispatches input event, we need to
> +    // wait for all fields are auto-filled first and then start handle the
> +    // highlight style for user might correct the filled fields afterward.

nit: "... wait for all fields to be autofilled first and then start to handle the highlight style resetting caused by user's correction afterward."

::: browser/extensions/formautofill/FormAutofillHandler.jsm:151
(Diff revision 4)
> +          if (fieldDetail.state === "AUTO_FILLED") {
> +            autofilledCount++;
> +          }

nit: I feel it doesn't need to be a counter. How about `hasFilledFields |= (fieldDetail.state === "AUTO_FILLED")`?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:156
(Diff revision 4)
> +          if (fieldDetail.state === "AUTO_FILLED") {
> +            autofilledCount++;
> +          }
> +        }
> +
> +        // Unregister listeners once no fields is in AUTO_FILLED state.

nit: "... no field is ..."
Attachment #8868955 - Flags: review?(lchang) → review+
Comment on attachment 8868956 [details]
Bug 1300996 - Part 4: Add mochitest for form autofill preview and highlight.

https://reviewboard.mozilla.org/r/140626/#review146328
Attachment #8868956 - Flags: review?(lchang) → review+
Comment on attachment 8861335 [details]
Bug 1300996 - Part 1: Set selectedIndex of autocomplete popup when mousemove over profile item.

https://reviewboard.mozilla.org/r/133322/#review146600

::: toolkit/components/satchel/test/browser/browser.ini:5
(Diff revision 9)
>  [browser_privbrowsing_perwindowpb.js]
> +[browser_popup_mouseover.js]

Nit: sort these alphabetically

::: toolkit/components/satchel/test/browser/browser_popup_mouseover.js:5
(Diff revision 9)
> +/* 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/. */
> +
> +const FormHistory = (Components.utils.import("resource://gre/modules/FormHistory.jsm", {})).FormHistory;

Why not just use:
Cu.import("resource://gre/modules/FormHistory.jsm");

I think Cu is already defined in browser-chrome tests.

::: toolkit/components/satchel/test/browser/browser_popup_mouseover.js:8
(Diff revision 9)
> +async function sendKeyEventToBrowser(browser, keyCode, modifier = null) {
> +  /* eslint no-shadow: ["error", { "allow": ["keyCode", "modifier"] }]*/
> +  await ContentTask.spawn(browser, {keyCode, modifier}, async function({keyCode, modifier}) {
> +    const utils = content.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> +                         .getInterface(Components.interfaces.nsIDOMWindowUtils);

IIUC, this is duplicating what BrowserTestUtils.synthesizeKey does. Is there a difference?

::: toolkit/components/satchel/test/browser/browser_popup_mouseover.js:11
(Diff revision 9)
> +    const utils = content.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> +                         .getInterface(Components.interfaces.nsIDOMWindowUtils);

s/Components\.interfaces/Ci/g

::: toolkit/components/satchel/test/browser/browser_popup_mouseover.js:23
(Diff revision 9)
> +  const tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, `data:text/html,<input type="text" name="field1">`, false);
> +  const browser = gBrowser.selectedBrowser;

This method looks like it would be good with `BrowserTestUtils.withNewTab`
Attachment #8861335 - Flags: review+
Comment on attachment 8861336 [details]
Bug 1300996 - Part 2: Show preview text on and highlight the fields that would be filled.

https://reviewboard.mozilla.org/r/133320/#review148108

Sorry for the delay

::: browser/extensions/formautofill/FormAutofillHandler.jsm:193
(Diff revision 9)
> +    for (let state in this.fieldStateEnum) {
> +      let mmState = this.fieldStateEnum[state];

Nit: Use `for…of` with `Object.entries` instead. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/entries

::: browser/extensions/formautofill/FormAutofillHandler.jsm:196
(Diff revision 9)
> +      // Do nothing if mmState is null, i.e. NORMAL state
> +      if (!mmState) {
> +        continue;

This comment isn't adding much value over reading the  code below it but it took me a why to understand *why* we are doing the `continue`. The comment should answer "why", not "what": The NORMAL state is simply the absensence of other manually managed states so we never need to add or remove it.
Attachment #8861336 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8868956 [details]
Bug 1300996 - Part 4: Add mochitest for form autofill preview and highlight.

https://reviewboard.mozilla.org/r/140626/#review148222

Some quick comments but I'll need to take another look when I'm not tired.

::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:9
(Diff revision 5)
>  
>  "use strict";
>  
>  let formFillChromeScript;
>  
>  function setInput(selector, value) {

Nit: add ` = undefined` after `value` to document that value is optional… though I don't see you using this anywhere so maybe revert this change?

::: browser/extensions/formautofill/test/mochitest/test_formautofill_preview_highlight.html:5
(Diff revision 5)
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <meta charset="utf-8">
> +  <title>Test basic autofill</title>

Nit: Update the title to describe the test since it shows on dxr file listings

::: browser/extensions/formautofill/test/mochitest/test_formautofill_preview_highlight.html:13
(Diff revision 5)
> +  <script type="text/javascript" src="formautofill_common.js"></script>
> +  <script type="text/javascript" src="satchel_common.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> +</head>
> +<body>
> +Form autofill test: simple form address autofill

Update this too
Comment on attachment 8868955 [details]
Bug 1300996 - Part 3: Handle the highlight style if the filled fields being changed by user.

https://reviewboard.mozilla.org/r/140624/#review148594

::: browser/extensions/formautofill/FormAutofillHandler.jsm:131
(Diff revision 5)
> +    // Since SetUserInput asynchronously dispatches input event, we need to
> +    // wait for all fields to be autofilled first and then start handle the
> +    // highlight style resetting caused by user's correction afterward.
> +    Promise.all(pendingFilledPromises).then(() => {

Are you sure it's asynchronous? I thought it was synchronous which would simplify this quite a bit
Comment on attachment 8868955 [details]
Bug 1300996 - Part 3: Handle the highlight style if the filled fields being changed by user.

https://reviewboard.mozilla.org/r/140624/#review148596
Attachment #8868955 - Flags: review?(MattN+bmo)
Whiteboard: [form autofill:M3] → [form autofill:M3] ETA:612
Comment on attachment 8861335 [details]
Bug 1300996 - Part 1: Set selectedIndex of autocomplete popup when mousemove over profile item.

https://reviewboard.mozilla.org/r/133322/#review146600

> IIUC, this is duplicating what BrowserTestUtils.synthesizeKey does. Is there a difference?

No difference after tested, thanks :)

> This method looks like it would be good with `BrowserTestUtils.withNewTab`

yeah, BrowserTestUtils.withNewTab works well.
Comment on attachment 8868955 [details]
Bug 1300996 - Part 3: Handle the highlight style if the filled fields being changed by user.

https://reviewboard.mozilla.org/r/140624/#review148594

> Are you sure it's asynchronous? I thought it was synchronous which would simplify this quite a bit

Sorry, I intuitively thought it's asynchronous...But still, we need to deal with asynchronous "input" event from focused input(FormFillController), so I added an one time listener to suppress it.
Comment on attachment 8868956 [details]
Bug 1300996 - Part 4: Add mochitest for form autofill preview and highlight.

https://reviewboard.mozilla.org/r/140626/#review148222

Thank you so much for all the reviews. 

I've updated the patches. Please help me to review part 3 and also have a quick look to others if you got some times. Thanks again.

> Nit: add ` = undefined` after `value` to document that value is optional… though I don't see you using this anywhere so maybe revert this change?

can't not recall why made this change :P 
reverted it.
Comment on attachment 8868955 [details]
Bug 1300996 - Part 3: Handle the highlight style if the filled fields being changed by user.

https://reviewboard.mozilla.org/r/140624/#review149540

::: browser/extensions/formautofill/FormAutofillHandler.jsm:125
(Diff revision 6)
> +          element.addEventListener("input", e => e.stopPropagation(), {
> +            once: true, mozSystemGroup: true,
> +          });

I'm not sure how `mozSystemGroup` affects things but normally we need to check e.isTrusted so that we don't react to synthesized DOM events created by webpages. Can you check if we nede that check here?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:137
(Diff revision 6)
> +    const onChangeHandler = e => {
> +      let hasFilledFields;
> +
> +      for (let fieldDetail of this.fieldDetails) {

I think this should also have a `e.isTrusted` check but I could be wrong due to `mozSystemGroup` being used.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:147
(Diff revision 6)
> +        if (e.target === element || e.type === "reset") {
> +          this.changeFieldState(fieldDetail, "NORMAL");
> +        }

For a reset event you should still check that `e.target == element.form` since the `reset` event bubbles and the root element can currently be a form ancestor (always the document currently) for <form>-less inputs. If a form gets reset, we shouldn't also change the field state of the form-less inputs since they shouldn't have been reset by DOM code.

Can you add a test case for this? Example:
```html
<input autocomplete="email">
<input autocomplete="given-name">
<input autocomplete="family-name">
<form>
  <input autocomplete="address-level1">
  <input autocomplete="address-level2">
  <input autocomplete="country">
  <input type="reset">
</form>
```

With your current patch I belive the reset button will reset the field state for the first 3 inputs even though the values don't get reset.
Attachment #8868955 - Flags: review?(MattN+bmo)
Comment on attachment 8868956 [details]
Bug 1300996 - Part 4: Add mochitest for form autofill preview and highlight.

https://reviewboard.mozilla.org/r/140626/#review149542

::: browser/extensions/formautofill/test/mochitest/test_formautofill_preview_highlight.html:51
(Diff revision 6)
> +// We could not get ManuallyManagedState of element now, so directly check if
> +// filter and text color style are applied.
> +function checkFieldPreview(elem, expectedText) {
> +  const computedStyle = window.getComputedStyle(elem);
> +  const isStyleApplied = computedStyle.getPropertyValue("filter") !== "none" &&
> +                         computedStyle.getPropertyValue("color") !== defaultTextColor;
> +
> +  is(SpecialPowers.wrap(elem).previewValue, expectedText, `Checking #${elem.id} previewValue`);
> +  is(isStyleApplied, !!expectedText, `Checking #${elem.id} preview style`);
> +}
> +
> +function checkFilledFieldHighlight(elem, expectedValue) {

We may want these in formautofill_common.js eventually but I guess it's okay here for now.

::: browser/extensions/formautofill/test/mochitest/test_formautofill_preview_highlight.html:64
(Diff revision 6)
> +  const isStyleApplied =  computedStyle.getPropertyValue("filter") !== "none" &&
> +                          computedStyle.getPropertyValue("color") === defaultTextColor;

Nit: extra space after `=`

::: browser/extensions/formautofill/test/mochitest/test_formautofill_preview_highlight.html:96
(Diff revision 6)
> +    const eventType = document.activeElement == element ? "DOMAutoComplete" : "change";
> +
> +    pendingPromises.push(new Promise(resolve => {
> +      element.addEventListener(eventType, resolve, {once: true});

Hmm… does the `change` event not fire on the active element? I thought this was fixed years ago but I do see some related bugs still open.

Oh… I guess it does fire but only when blurred since that's when change normally fires for a focused element. Perhaps we should only listen for "change" but have check_filled_highlight change the focus after hitting return?

::: browser/extensions/formautofill/test/mochitest/test_formautofill_preview_highlight.html:160
(Diff revision 6)
> +    <p><label>organization: <input id="organization" name="organization" autocomplete="organization" type="text"></label></p>
> +    <p><label>streetAddress: <input id="street-address" name="street-address" autocomplete="street-address" type="text"></label></p>
> +    <p><label>tel: <input id="tel" name="tel" autocomplete="tel" type="text"></label></p>
> +    <p><label>country: <input id="country" name="country" autocomplete="country" type="text"></label></p>

Nit: I personally would remove non-essential attributes such as @name and @type to make the test more readable.
Attachment #8868956 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8861336 [details]
Bug 1300996 - Part 2: Show preview text on and highlight the fields that would be filled.

https://reviewboard.mozilla.org/r/133320/#review149544

::: browser/extensions/formautofill/FormAutofillHandler.jsm:195
(Diff revisions 9 - 10)
>        log.warn(fieldDetail.fieldName, "is trying to change to an invalid state");
>        return;
>      }
>  
> -    for (let state in this.fieldStateEnum) {
> -      let mmState = this.fieldStateEnum[state];
> +    for (let [state, mmStateValue] of Object.entries(this.fieldStateEnum)) {
> +      // The NORMAL state is simply the absensence of other manually

typo: absence (possibly my fault)
Comment on attachment 8868955 [details]
Bug 1300996 - Part 3: Handle the highlight style if the filled fields being changed by user.

https://reviewboard.mozilla.org/r/140624/#review149540

> I'm not sure how `mozSystemGroup` affects things but normally we need to check e.isTrusted so that we don't react to synthesized DOM events created by webpages. Can you check if we nede that check here?

Yeah, no mozSystemGroup is needed as autocomplete is already chrome privileged. Still need to check e.isTrusted no matter which type of listener we use.

> For a reset event you should still check that `e.target == element.form` since the `reset` event bubbles and the root element can currently be a form ancestor (always the document currently) for <form>-less inputs. If a form gets reset, we shouldn't also change the field state of the form-less inputs since they shouldn't have been reset by DOM code.
> 
> Can you add a test case for this? Example:
> ```html
> <input autocomplete="email">
> <input autocomplete="given-name">
> <input autocomplete="family-name">
> <form>
>   <input autocomplete="address-level1">
>   <input autocomplete="address-level2">
>   <input autocomplete="country">
>   <input type="reset">
> </form>
> ```
> 
> With your current patch I belive the reset button will reset the field state for the first 3 inputs even though the values don't get reset.

You're right, current patch does reset the fields for the first 3 inputs. And the issue is fixed after adding `e.target == element.form` check
Comment on attachment 8868956 [details]
Bug 1300996 - Part 4: Add mochitest for form autofill preview and highlight.

https://reviewboard.mozilla.org/r/140626/#review149542

Updated the patches including fixed eslint. Thank you for the review.

> Hmm… does the `change` event not fire on the active element? I thought this was fixed years ago but I do see some related bugs still open.
> 
> Oh… I guess it does fire but only when blurred since that's when change normally fires for a focused element. Perhaps we should only listen for "change" but have check_filled_highlight change the focus after hitting return?

The 'change' fires even without blur the focused input. Now listen to "change" only in the revision.

> Nit: I personally would remove non-essential attributes such as @name and @type to make the test more readable.

I agree with it. I've removed those.
Comment on attachment 8868955 [details]
Bug 1300996 - Part 3: Handle the highlight style if the filled fields being changed by user.

https://reviewboard.mozilla.org/r/140624/#review149540

> You're right, current patch does reset the fields for the first 3 inputs. And the issue is fixed after adding `e.target == element.form` check

> Can you add a test case for this?

Please file a follow-up to add a test for this case.
Comment on attachment 8868955 [details]
Bug 1300996 - Part 3: Handle the highlight style if the filled fields being changed by user.

https://reviewboard.mozilla.org/r/140624/#review150500

::: browser/extensions/formautofill/FormAutofillHandler.jsm:172
(Diff revision 9)
> +        if (e.target == element || (e.target == element.form && e.type === "reset")) {
> +          this.changeFieldState(fieldDetail, "NORMAL");
> +        }
> +
> +        hasFilledFields |= (fieldDetail.state === "AUTO_FILLED");

Nit: use double quotes x2
Attachment #8868955 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8868955 [details]
Bug 1300996 - Part 3: Handle the highlight style if the filled fields being changed by user.

https://reviewboard.mozilla.org/r/140624/#review150500

> Nit: use double quotes x2

Sorry, I meant double-equals
Blocks: 1370798
It seems a bit buggy for part 1 mozreview, there's no way to resolve the remaining 1 issue that should has been resolved, so probably need manually land it. Matt, could you help with this? Thanks
Keywords: checkin-needed
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f54fca58d189
Part 1: Set selectedIndex of autocomplete popup when mousemove over profile item. r=adw
https://hg.mozilla.org/integration/mozilla-inbound/rev/da12ceebe125
Part 2: Show preview text on and highlight the fields that would be filled. r=MattN, lchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/71ef10fe925e
Part 3: Handle the highlight style if the filled fields being changed by user. r=MattN, lchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/221b17bc902a
Part 4: Add mochitest for form autofill preview and highlight. r=MattN, lchang
Keywords: checkin-needed
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/baea1f6031e8
Part 1: Set selectedIndex of autocomplete popup when mousemove over profile item. r=adw
https://hg.mozilla.org/integration/mozilla-inbound/rev/97543ecd15b6
Part 2: Show preview text on and highlight the fields that would be filled. r=MattN, lchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/20b16e014c00
Part 3: Handle the highlight style if the filled fields being changed by user. r=MattN, lchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/bacd825eed43
Part 4: Add mochitest for form autofill preview and highlight. r=MattN, lchang
Since previewValue is a chrome-only API used internally, plus we have an early return check for mPreviewDiv existence, I think it's safe to remove the assertion that causes the xpcshell failure. Still, I haven't figured out why previewValue throws error only in xpcshell, but I'd prefer split off another followup for that and don't block current patch if no major harm.

Remove the assertion here:
http://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/dom/html/nsTextEditorState.cpp#2820

The try result without assertion:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d76cc4be51cb1c77c50ce1a2c7692bb4e912acd0&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable
Flags: needinfo?(ralin)
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c98b13c5b45
Part 1: Set selectedIndex of autocomplete popup when mousemove over profile item. r=adw
https://hg.mozilla.org/integration/mozilla-inbound/rev/13b52285f52c
Part 2: Show preview text on and highlight the fields that would be filled. r=MattN, lchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bac6640f258
Part 3: Handle the highlight style if the filled fields being changed by user. r=MattN, lchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/587d7e9b6c7e
Part 4: Add mochitest for form autofill preview and highlight. r=MattN, lchang
Depends on: 1374550
Regressions: 1567918
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: