974745 - Create click event on Sponsored Tiles to show explanation panel

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Jennifer Morrow [:Boriss] (UX)]

This hover event spawns from Sponsored Tile icons (added in bug 974736).  The panel created briefly explains what Sponsored Tiles are and links to a Mozilla Public Page providing more information (page created in bug 974728).

Future version contain have additional information and features on this panel, but this basic first version only gives text a link.

Acceptance criteria: 

1. On mouseover with standard tooltip delay, the a Sponsored Tile panel is displayed
2. On cessation of mouseover, the panel fades quickly and disappears
3. On click, the panel displays and maintains visibility even if the user ceases mouseover.  Panel is only dismissed from this state if user clicks else (in New Tab page, on the icon itself, or in Firefox chrome)
4. On Mozilla Public Page link click, the Mozilla Public Page explaining Sponsored Tiles opens in the current tab.

Comment 1

[Ed Lee :Mardak]

What should happen if there is not enough screen space at the bottom to show the panel? The mockup has the panel with an arrow pointing at the sponsored icon (similar to the doorhanger in Firefox). I see that the doorhanger does tries to keep the arrow pointing upwards/downwards towards the left/right side of the panel as it can fit. We should mimic that behavior?

Comment 2

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Edward Lee :Mardak from comment #1)
> What should happen if there is not enough screen space at the bottom to show
> the panel? The mockup has the panel with an arrow pointing at the sponsored
> icon (similar to the doorhanger in Firefox). I see that the doorhanger does
> tries to keep the arrow pointing upwards/downwards towards the left/right
> side of the panel as it can fit. We should mimic that behavior?

Exactly right - it should mimic the behavior of the doorhanger, which in this case would mainly appear upwards of the icon.

Comment 3

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Panel displaying on hover event

Comment 4

[Ed Lee :Mardak]

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #0)
> 2. On cessation of mouseover, the panel fades quickly and disappears
So just to be clear, even if the user tries to click the "Learn more..." link, it should fade unless the user clicked the icon to make it persist? Or should there be a delay so that if the user moves into the panel within that time, the panel will persist?

Comment 5

[Ed Lee :Mardak]

We might be running into bug 962124 because the arrow panel transitions in (shifting down) as it appears, so that's why the mouse enters the panel when it first appears.

Comment 6

[Ed Lee :Mardak]

Bug 956162 recently landed implementing @flip=none which should prevent the animation but arrowpanels are hardcoded to @flip=both. Although I'm not sure if we actually want @flip=none because it'll prevent the logic from attempting to keep the panel visible on screen.

Comment 7

[Jennifer Morrow [:Boriss] (UX)]

As noted in bug 974736#c29, this is changing from a hover event to a click event.

(In reply to Ed Lee :Mardak from comment #4)
> (In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #0)
> > 2. On cessation of mouseover, the panel fades quickly and disappears
> So just to be clear, even if the user tries to click the "Learn more..."
> link, it should fade unless the user clicked the icon to make it persist? Or
> should there be a delay so that if the user moves into the panel within that
> time, the panel will persist?

With this as a click event, the panel should fade if the user clicks anywhere outside of the panel.  Panel will be persistent until next click and is activated via icon click.

Comment 8

[Ed Lee :Mardak]

Attachment: v1

adw, a couple questions regarding this patch:

1) should the panel's xul:label use value=&text instead of >&text</
2) should we add a new sponsoredPanel.js that knows how to show instead of having it part of Site as Site_showSponsoredPanel

Comment 9

[Ed Lee :Mardak]

Attachment: v2

Simplified the panel structure and fixed styling per Boriss' comments

Comment 10

[Drew Willcoxon :adw]

Comment on attachment 8387440 [details] [diff] [review]
v2

Review of attachment 8387440 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK to me, except I think it'd be better to define showSponsoredPanel on gPage in page.js instead of on Site.prototype, since the panel is a property of the page, not each site, even though the panel pops up on sites.  Site.prototype._onClick would still handle the click but pass it off to gPage.showSponsoredPanel.  Sound OK?

::: browser/base/content/newtab/sites.js
@@ +226,5 @@
>  
>    /**
> +   * Opens sponsored button panel
> +   */
> +  showSponsoredPanel: function Site_showSponsoredPanel(target) {

Nit: aTarget to match the style in the newtab code.

@@ +229,5 @@
> +   */
> +  showSponsoredPanel: function Site_showSponsoredPanel(target) {
> +    if (this._sponsoredPanel.state == "closed") {
> +      this._sponsoredPanel.addEventListener("popuphidden", function onPopupHidden(e) {
> +        this._sponsoredPanel.removeEventListener("popuphidden", onPopupHidden, false);

This doesn't actually remove the listener, because the listener is the function returned by bind(), not onPopupHidden, right?

Comment 11

[Drew Willcoxon :adw]

(In reply to Ed Lee :Mardak from comment #8)
> Created attachment 8386893 [details] [diff] [review]
> v1
> 
> adw, a couple questions regarding this patch:
> 
> 1) should the panel's xul:label use value=&text instead of >&text</
> 2) should we add a new sponsoredPanel.js that knows how to show instead of
> having it part of Site as Site_showSponsoredPanel

Missed these.

1) The value attribute is fine.
2) Think I inadvertently addressed this.  I don't think a new file is necessary.

Comment 12

[Ed Lee :Mardak]

(In reply to Drew Willcoxon :adw from comment #10)
> Looks OK to me, except I think it'd be better to define showSponsoredPanel
> on gPage in page.js instead of on Site.prototype, since the panel is a
> property of the page, not each site, even though the panel pops up on sites.
> Site.prototype._onClick would still handle the click but pass it off to
> gPage.showSponsoredPanel.  Sound OK?
Sounds great. Thanks.

> > +      this._sponsoredPanel.addEventListener("popuphidden", function onPopupHidden(e) {
> > +        this._sponsoredPanel.removeEventListener("popuphidden", onPopupHidden, false);
> This doesn't actually remove the listener, because the listener is the
> function returned by bind(), not onPopupHidden, right?
Ah nice catch. I explicitly said not to use => because arguments.callee wouldn't work correctly for remove. I believe the pattern is to just assign the bound handler and use that for both add/remove. Or do the let self = this and use anonymous/arguments.callee.

Comment 13

[maxim zhilyaev]

Attachment: v3

Moved showSponsoredPanel logic into page.js

Comment 14

[maxim zhilyaev]

Attachment: V3.1

Clarified css comment.

Comment 15

[Drew Willcoxon :adw]

Comment on attachment 8389433 [details] [diff] [review]
V3.1

Review of attachment 8389433 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/newtab/sites.js
@@ +213,5 @@
>  
>      aEvent.preventDefault();
> +    if (target.classList.contains("newtab-control-sponsored"))
> +      gPage.showSponsoredPanel(target);
> +    else if (target.classList.contains("newtab-control-block"))

Nit: Would you mind adding your new conditional after the existing if, as an else if?  That way the patch doesn't change more lines than necessary, and blame is preserved for the existing if.

Comment 16

[Ed Lee :Mardak]

Attachment: for checkin

Comment 17

[u279076]

I think this should be covered by automation.

Comment 18

[Ed Lee :Mardak]

maksik: we'll want a unit test for synthesizing a mouse event on the icon and checking if the panel appears then disappears when clicking away

Comment 19

[Ed Lee :Mardak]

Attachment: for checkin

Adds test and emtwo's fix for styling the sponsored icon as 14x14 instead of 16x16.

Comment 20

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/5aed75c602ce

Comment 21

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/4caa44af6d1b, see bug 975228 comment 30.

Comment 22

[Phil Ringnalda (:philor)]

And also, though I didn't notice it until afterward, because browser_newtab_sponsored_icon_click.js timed out on Windows, https://tbpl.mozilla.org/php/getParsedLog.php?id=36941162&tree=Fx-Team https://tbpl.mozilla.org/php/getParsedLog.php?id=36941198&tree=Fx-Team https://tbpl.mozilla.org/php/getParsedLog.php?id=36941230&tree=Fx-Team

Comment 23

[Ed Lee :Mardak]

maksik, the test seems to only work on Mac and is failing on Linux and Windows. I'm guessing it's related to how you generate mouse down/up events.

Comment 24

[Ed Lee :Mardak]

Attachment: for checkin

Avoids Linux and Windows test failures by closing the popup and checking for correct states instead of relying on clicks to dismiss.

Comment 25

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/3f245df4a3cd

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in http://hg.mozilla.org/integration/fx-team/rev/4d8323b1389d for frequently making Jetpack tests fail with https://tbpl.mozilla.org/php/getParsedLog.php?id=37024180&tree=Fx-Team

Comment 27

[Ed Lee :Mardak]

The test failing: http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/test/test-content-events.js#81

TEST-UNEXPECTED-FAIL | tests/test-content-events.test nested frames | events where dispatched - [
    "document-element-inserted -> data:text/html,%3Ciframe%20src='data:text/html,iframe'%3E",
    "DOMContentLoaded -> data:text/html,%3Ciframe%20src='data:text/html,iframe'%3E",
    "document-element-inserted -> data:text/html,iframe",
    "DOMContentLoaded -> data:text/html,iframe",
    "load -> data:text/html,iframe",
    "pageshow -> data:text/html,iframe",
    "load -> data:text/html,%3Ciframe%20src='data:text/html,iframe'%3E",
    "pageshow -> data:text/html,%3Ciframe%20src='data:text/html,iframe'%3E"
] deepEqual [
    "document-element-inserted -> data:text/html,%3Ciframe%20src='data:text/html,iframe'%3E",
    "DOMContentLoaded -> data:text/html,%3Ciframe%20src='data:text/html,iframe'%3E",
    "document-element-inserted -> chrome://global/skin/arrow/panelarrow-vertical.svg",
    "document-element-inserted -> data:text/html,iframe",
    "DOMContentLoaded -> data:text/html,iframe",
    "load -> data:text/html,iframe",
    "pageshow -> data:text/html,iframe",
    "load -> data:text/html,%3Ciframe%20src='data:text/html,iframe'%3E",
    "pageshow -> data:text/html,%3Ciframe%20src='data:text/html,iframe'%3E"
]

It sees a document-element-inserted with panelarrow-vertical.svg... on some runs. I'm guessing maybe it wasn't fully loaded from a previous test that did open a new tab page?

Comment 28

[Ed Lee :Mardak]

The test seems to also fail sometimes for this one: http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/test/test-content-events.js#39

TEST-UNEXPECTED-FAIL | tests/test-content-events.test multiple tabs | all events dispatche as expeced - [
    "document-element-inserted -> data:text/html,first-tab",
    "DOMContentLoaded -> data:text/html,first-tab",
    "load -> data:text/html,first-tab",
    "pageshow -> data:text/html,first-tab",
    "document-element-inserted -> data:text/html,second-tab",
    "DOMContentLoaded -> data:text/html,second-tab",
    "load -> data:text/html,second-tab",
    "pageshow -> data:text/html,second-tab"
] deepEqual [
    "document-element-inserted -> chrome://global/skin/arrow/panelarrow-vertical.svg",
    "document-element-inserted -> data:text/html,first-tab",
    "DOMContentLoaded -> data:text/html,first-tab",
    "load -> data:text/html,first-tab",
    "pageshow -> data:text/html,first-tab",
    "document-element-inserted -> data:text/html,second-tab",
    "DOMContentLoaded -> data:text/html,second-tab",
    "load -> data:text/html,second-tab",
    "pageshow -> data:text/html,second-tab"
]

Comment 29

[Ed Lee :Mardak]

Relanded behind bug 990284 fix https://hg.mozilla.org/integration/fx-team/rev/f3fa2a144839

https://hg.mozilla.org/integration/fx-team/rev/f3c44cfc3900

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f3c44cfc3900