Closed
Bug 1270953
Opened 9 years ago
Closed 9 years ago
Enhancements for notification popups
Categories
(Testing :: Firefox UI Tests, defect)
Tracking
(firefox48 fixed, firefox49 fixed)
RESOLVED
FIXED
mozilla49
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
Attachments
(3 files)
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•9 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•9 years ago
|
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 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•9 years ago
|
||
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•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/53242/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53242/
Assignee | ||
Comment 5•9 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•9 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 | ||
Comment 7•9 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!
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•9 years ago
|
||
Comment 11•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/90cad97e0b06
https://hg.mozilla.org/mozilla-central/rev/d1d706fd85f4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 12•8 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]
Comment 13•8 years ago
|
||
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•8 years ago
|
||
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•8 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]
Comment 16•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/c2d6ad2297e3
and
https://hg.mozilla.org/releases/mozilla-aurora/rev/868634071db22a114c8053c139028c7766f34bcf
Whiteboard: [checkin-needed-aurora]
You need to log in
before you can comment on or make changes to this bug.
Description
•