Enhancements for notification popups

RESOLVED FIXED in Firefox 48

Status

Testing
Firefox UI Tests
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

Version 3
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed, firefox49 fixed)

Details

MozReview Requests

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

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
Follow-up bug for my last comment on bug 1144873. For ease of discovery here again:

https://reviewboard.mozilla.org/r/45279/#review47709

Hey Dave! Thanks a ton for working on the implementation for doorhanger notifications! It's great to see that those have been landed now. 

Even this bug is fixed I checked the code and made some comments. There is nothing we have to work on this bug, but given the comment Bugzilla will create, I will file new bug for enhancements to this patch.

::: testing/firefox-ui/tests/puppeteer/test_notifications.py:75
(Diff revision 2)
> +        with self.assertRaisesRegexp(TimeoutException, message):
> +            self.browser.wait_for_notification(None)
> +
> +    def trigger_add_on_notification(self, add_on):
> +        with self.marionette.using_context('content'):
> +            self.marionette.navigate('file://{0}'.format(here))

For resources we have a separate folder under http://mxr.mozilla.org/mozilla-central/source/testing/firefox-ui/resources/. It should have been the place where the XPI is stored. We can fix that in a follow-up bug. Also we should not install via file:// but localhost://

::: testing/puppeteer/firefox/firefox_puppeteer/ui/browser/notifications.py:5
(Diff revision 2)
> +# 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/.
> +
> +from abc import ABCMeta

We haven't used the meta classes anywhere else yet. Introducing them somewhere in the middle of the class hierachy is a bit irritating. But lets get a new bug filed to get the basic classes updated.

::: testing/puppeteer/firefox/firefox_puppeteer/ui/browser/notifications.py:32
(Diff revision 2)
> +
> +        :returns: The notification origin.
> +        """
> +        return self.element.get_attribute('origin')
> +
> +    def close(self):

We need a `force` option here, so we can ensure at least in teardown that the notification gets definitely closed.

::: testing/puppeteer/firefox/firefox_puppeteer/ui/browser/notifications.py:35
(Diff revision 2)
> +        return self.element.get_attribute('origin')
> +
> +    def close(self):
> +        """Close the notification."""
> +        self.element.find_element(
> +            By.ANON_ATTRIBUTE, {'anonid': 'closebutton'}).click()

Usually we have a separate property for elements, so we can act on them directly and not via helper methods.

::: testing/puppeteer/firefox/firefox_puppeteer/ui/browser/notifications.py:45
(Diff revision 2)
> +
> +    def allow(self):
> +        """Allow the add-on to be installed."""
> +        self.element.find_element(
> +            By.ANON_ATTRIBUTE, {'anonid': 'button'}).find_element(
> +            By.ANON_ATTRIBUTE, {'anonid': 'button'}).click()

Same here for the element, including other instances below.

::: testing/puppeteer/firefox/firefox_puppeteer/ui/browser/window.py:97
(Diff revision 2)
>              self._navbar = NavBar(lambda: self.marionette, self, navbar)
>  
>          return self._navbar
>  
>      @property
> +    def notification(self):

Also we might have to think about if the notification directly under the browser is correct here. UI wise it is connected to the toolbar, and even only available for the tab where it has been raised. But maybe we should keep the door hanger notifications as is and add the tab-specific notifications under the tab.

::: testing/puppeteer/firefox/firefox_puppeteer/ui/browser/window.py:115
(Diff revision 2)
> +                By.CSS_SELECTOR, '#notification-popup popupnotification')
> +        except NoSuchElementException:
> +            return None  # no notification is displayed
> +        notification_id = notification.get_attribute('id')
> +        return notifications_map[notification_id](
> +            lambda: self.marionette, self, notification)

I would propose that we return a base notification in case the current type is not being supported yet.

::: testing/puppeteer/firefox/firefox_puppeteer/ui/browser/window.py:118
(Diff revision 2)
> +        notification_id = notification.get_attribute('id')
> +        return notifications_map[notification_id](
> +            lambda: self.marionette, self, notification)
> +
> +    def wait_for_notification(self, notification_class=BaseNotification):
> +        """Waits for the specified notification to be displayed."""

nit: we should always document the parameters for the right API documentation.

::: testing/puppeteer/firefox/firefox_puppeteer/ui/browser/window.py:126
(Diff revision 2)
> +                lambda _: self.notification is None,
> +                message='Unexpected notification shown.')
> +        else:
> +            message = '{0} was not shown.'.format(notification_class.__name__)
> +            if notification_class is BaseNotification:
> +                message = 'No notification was shown.'

Not sure why we have to set the message again. The `__name__` property should also be existent for the BaseNotification class.
(Assignee)

Comment 1

2 years ago
I have started to work on this bug but run into a problem with the add-on installation. Now that the test tries to install it via the local webserver from Marionette I do not get the notification that the install is blocked due to no verification. Instead the addon gets installed successfully. I tried the same add-on with mozqa.com and there I get the blocked notification.

Is there something special about localhost? Dave, could you help us? Thanks.
Flags: needinfo?(dtownsend)
(Assignee)

Updated

2 years ago
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Blocks: 1271911
(Assignee)

Comment 2

2 years ago
I'm not sure how to fix the test failure on bug 1271911 which only happens with inbound builds but not nightlies. I will move the request from Dave forward to the other bug, and skip the test for now.
No longer blocks: 1271911
Flags: needinfo?(dtownsend)
(Assignee)

Comment 3

2 years ago
Created attachment 8753373 [details]
MozReview Request: Bug 1270953 - Enhance utils module for permissions. r?maja_zf

Review commit: https://reviewboard.mozilla.org/r/53240/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53240/
Attachment #8753373 - Flags: review?(mjzffr)
Attachment #8753374 - Flags: review?(mjzffr)
(Assignee)

Comment 4

2 years ago
Created attachment 8753374 [details]
MozReview Request: Bug 1270953 - Enhance notification class for Firefox Puppeteer. r?maja_zf

Review commit: https://reviewboard.mozilla.org/r/53242/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53242/
(Assignee)

Comment 5

2 years ago
Comment on attachment 8753373 [details]
MozReview Request: Bug 1270953 - Enhance utils module for permissions. r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53240/diff/1-2/
(Assignee)

Comment 6

2 years ago
Comment on attachment 8753374 [details]
MozReview Request: Bug 1270953 - Enhance notification class for Firefox Puppeteer. r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53242/diff/1-2/
(Assignee)

Updated

2 years ago
Blocks: 1271911
(Assignee)

Comment 7

2 years ago
The before mentioned test failure as a bustage locally. A rebuilt of my fragment build fixed it. So we do not have to skip it!
(Assignee)

Updated

2 years ago
Blocks: 1272710
(Assignee)

Updated

2 years ago
Blocks: 1272709
Comment on attachment 8753373 [details]
MozReview Request: Bug 1270953 - Enhance utils module for permissions. r?maja_zf

https://reviewboard.mozilla.org/r/53240/#review50088
Attachment #8753373 - Flags: review?(mjzffr) → review+
Comment on attachment 8753374 [details]
MozReview Request: Bug 1270953 - Enhance notification class for Firefox Puppeteer. r?maja_zf

https://reviewboard.mozilla.org/r/53242/#review50094
Attachment #8753374 - Flags: review?(mjzffr) → review+

Comment 10

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/90cad97e0b06
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1d706fd85f4
https://hg.mozilla.org/mozilla-central/rev/90cad97e0b06
https://hg.mozilla.org/mozilla-central/rev/d1d706fd85f4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Updated

2 years ago
No longer blocks: 1271911
(Assignee)

Comment 12

2 years ago
Since the patch landed on mozilla-central the number of different failures went down to only 2. So I would like to see a backport of those test changes to mozilla-aurora too.
status-firefox48: --- → affected
Whiteboard: [checkin-needed-aurora]
Hi Henrik this fails to apply on aurora:

Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r 90cad97e0b06
grafting 345230:90cad97e0b06 "Bug 1270953 - Enhance utils module for permissions. r=maja_zf"
merging testing/puppeteer/firefox/firefox_puppeteer/api/utils.py
warning: conflicts while merging testing/puppeteer/firefox/firefox_puppeteer/api/utils.py! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ vim testing/puppeteer/firefox/firefox_puppeteer/api/utils.py

could you take a look ?
Flags: needinfo?(hskupin)
Whiteboard: [checkin-needed-aurora]
(Assignee)

Comment 14

2 years ago
Created attachment 8755823 [details] [diff] [review]
Backport patch for attachment 8753373 [details]

Backport patch for aurora without any code changes. Just an update of the lines because a method from mozilla-central is not available on mozilla-aurora.
Flags: needinfo?(hskupin)
Attachment #8755823 - Flags: review+
(Assignee)

Comment 15

2 years ago
Carsten, the conflict has been fixed. What we would need to land are attachment 8755823 [details] [diff] [review] and changeset d1d706fd85f4. Thanks.
Whiteboard: [checkin-needed-aurora]
 https://hg.mozilla.org/releases/mozilla-aurora/rev/c2d6ad2297e3
and
https://hg.mozilla.org/releases/mozilla-aurora/rev/868634071db22a114c8053c139028c7766f34bcf
status-firefox48: affected → fixed
Whiteboard: [checkin-needed-aurora]
You need to log in before you can comment on or make changes to this bug.