1838295 - Create Feature Callout fallback anchor

Status: RESOLVED FIXED

(Firefox :: Messaging System, enhancement, P1)

Description

[Jason Prickett]

To prevent issues in cases where the feature callout is anchored to an element that may not exist, we should allow for the use of a fallback anchor, and perhaps a fallback absolute position. This should come in the form of a parameter in the message such as fallback_parent_selector, or something similar. The feature callout should first check to see if it's initial parent selector exists, and if it does not, it should revert to using the fallback anchor defined in message. If neither element exists, the feature callout should not be displayed.

Comment 1

[Shane Hughes [:aminomancer]]

Good idea about fallback position. Maybe we can do something like this

interface fallback_anchor {
  parent_selector?: string,
  arrow_position: string,
  hide_arrow?: boolean,
  callout_position_override?: {
    top?: string,
    right?: string,
    bottom?: string,
    left?: string
  }
}

content: {
  position: "callout",
  fallback_anchor: {
    parent_selector: "PanelUI-menu-button",
    arrow_position: "top-end",
  }
}

Comment 2

[Shane Hughes [:aminomancer]]

Given we don't have any off-train feature callout messages yet, since the system doesn't support them, and all experimentation is just changing pref values, I suppose we could actually change the existing positioning schema. And have both use the same structure. Seems less confusing than having regular anchor properties under content, and having fallback anchor properties under content.fallback_anchor.

interface anchor {
  parent_selector?: string,
  arrow_position?: string,
  hide_arrow?: boolean,
  callout_position_override?: {
    top?: string,
    right?: string,
    bottom?: string,
    left?: string
  }
}

content: {
  position: "callout",
  anchor: {
    parent_selector: "some-conditional-urlbar-button",
    arrow_position: "top",
  },
  fallback_anchor: {
    hide_arrow: true,
    callout_position_override: {
      top: "45px",
      right: "120px"
    }
  }
}

Comment 3

[Jason Prickett]

Agreed. That's definitely cleaner and easier to read, makes more sense logically, and will help to prevent writing errors in our messages.

Comment 4

[yozhang]

After talking to [:aminomancer], it was suggested that an array of fallback anchors would be an even better improvement instead of simply having only one fallback anchor, ex:

content: {
    anchors: [
      {
        parent_selector?: string,
        arrow_position?: string,
        hide_arrow?: boolean,
        callout_position_override?: object,
      },
      { /* fallback 1 */ },
      { /* fallback 2 */ },
      { /* fallback 3 */ },
    ]
}

Comment 5

[yozhang]

Attachment: Bug 1838295 - Implementing Feature Callout Fallback Anchor r=jprickett,aminomancer

Comment 6

[yozhang]

Handing off to Shane after the recent patch change for Messages

Comment 7

[Shane Hughes [:aminomancer]]

Attachment: Bug 1838295 - Feature Callout fallback anchor system. r=jprickett

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f0a2e1e42f5
Feature Callout fallback anchor system. r=jprickett

Comment 9

[Cristian Tuns]

Backed out for causing bc failures in browser_feature_callout_in_chrome.js

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | browser/components/newtab/test/browser/browser_feature_callout_in_chrome.js | Uncaught exception in test bound triggered_feature_tour_with_custom_pref - undefined - timed out after 50 tries.

Fail 2: TEST-UNEXPECTED-FAIL | browser/components/newtab/test/browser/browser_feature_callout_in_chrome.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/browser.xhtml]
Log 2: https://treeherder.mozilla.org/logviewer?job_id=423513622&repo=autoland

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/829bb0f62051
Feature Callout fallback anchor system. r=jprickett

Comment 11

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/829bb0f62051