Closed Bug 1420574 Opened 4 years ago Closed 4 years ago

Clarify that browser_oneOffHeader.js fails if screen resolution is small

Categories

(Firefox :: Search, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(1 file, 3 obsolete files)

browser_oneOffHeader.js enables search bar, and uses searchbar-search-button there,
but if the screen resolution is small (this may happen while testing on VM), the search bar overflows and the button isn't created.

it should be nice to check the existence at the beginning, and fail with helpful message.
This fails locally with linux VM on VirtualBox, with default resolution (1024x768).
Added message that clarifies the reason.
Attachment #8931835 - Flags: review?(mak77)
Comment on attachment 8931835 [details] [diff] [review]
Clarify that browser_oneOffHeader.js fails if screen resolution is small.

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

The patch may even be fine, though there are a lot of other tests setting browser.search.widget.inNavBar to true, and if we should patch every one of them, it may be a nightmare.

Would it work if we'd temporarily set the overflows="false" attribute on "search-container" for the duration of the test?
That could be simpler to port to other tests.

Also, maybe Paolo has ideas on how to have a more general solution.
Attachment #8931835 - Flags: review?(mak77)
Flags: needinfo?(paolo.mozmail)
Thanks!

(In reply to Marco Bonardo [::mak] from comment #2)
> Would it work if we'd temporarily set the overflows="false" attribute on
> "search-container" for the duration of the test?
> That could be simpler to port to other tests.

Just adding the attribute doesn't move the search-container out of overflow menu.
but setting browser.search.widget.inNavBar to false and then true moves it to toolbar.
so I think the attribute needs to be set before the pref is set to true, but search-container element doesn't exist at the beginning.

I'll look for a bit more easy way.
another question is, whether this is really expected state.
several testcases expect search bar to be visible once the pref is set,
but search bar surely can overflow when screen resolution is small (I'm not sure if 1024x768 is *really* small these days)

now I'm wondering if it's something needs to be fixed on searchbar or toolbar implementation.
especially, there are UI tour testcases that expects searchbar to be visible, that seems to be a bad sign.
I'm not sure if all of these tests require the search bar to be visible, but for those that do, it is certainly better to check  in advance that the search bar didn't overflow. We could definitely use a helper function to remove duplication.

Off the top of my head, I don't expect this to be hiding production bugs. We have separate tests for the overflow case.

Switching "browser.search.widget.inNavBar" twice is equivalent to changing the related option from the preferences, and it is expected that it might change the position of the widget. Unless there are user-visible bugs that really break the behavior when this is done on small windows, we don't need to care about this.
Flags: needinfo?(paolo.mozmail)
Marking P1 because there's a patch. Is this easy to finish up?
Priority: -- → P1
The remaining things to do here is:
  * list up the tests that touches browser.search.widget.inNavBar
  * list up the tests that expects search bar to be actually visible, among above tests
  * add helper function that enables search bar, and check if it's visible if necessary (if the test expects the search bar is visible)
  * use the helper function in above tests

maybe it takes 1-2 days.
Flags: needinfo?(arai.unmht)
Here's the list of tests that touches browser.search.widget.inNavBar, and the result of the test with small screen.

* browser/base/content/test/about/browser_aboutHome_search_searchbar.js
  fails because searchbar's textbox is not found

* browser/components/customizableui/test/browser_694291_searchbar_preference.js
  OK (does not touch search bar itself)
* browser/components/customizableui/test/browser_901207_searchbar_in_panel.js
  fails because searchbar.textbox doesn't become active
  (this file is testing overflow with various width,
   and it expects the searchbar does not overflow with default width)
* browser/components/customizableui/test/browser_customization_context_menus.js
  fails because animation for searchbar doesn't happen

* browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js
  fails because searchbar's button is not found

* browser/components/search/test/browser_426329.js
  fails because searchbar's button is not found
* browser/components/search/test/browser_google_behavior.js
  fails because searchbar cannot get input
* browser/components/search/test/browser_healthreport.js
  fails because histogram for the search engine is not updated
* browser/components/search/test/browser_hiddenOneOffs_diacritics.js
  fails because searchbar's button is not found
* browser/components/search/test/browser_oneOffContextMenu.js
  fails because searchbar's button is not found
* browser/components/search/test/browser_oneOffContextMenu_setDefault.js
  fails because searchbar's button is not found
* browser/components/search/test/browser_oneOffHeader.js
  fails because searchbar's button is not found
* browser/components/search/test/browser_private_search_perwindowpb.js
  fails because searchbar cannot get input
* browser/components/search/test/browser_searchEngine_behaviors.js
  fails because searchbar cannot get input
* browser/components/search/test/browser_searchbar_keyboard_navigation.js
  fails because searchbar's FormHistory is not found
* browser/components/search/test/browser_searchbar_openpopup.js
  fails because searchbar's button is not found
* browser/components/search/test/browser_searchbar_smallpanel_keyboard_navigation.js
  fails because searchbar's button is not found
* browser/components/search/test/browser_tooManyEnginesOffered.js
  fails because searchbar cannot get input

* browser/components/uitour/test/browser_UITour.js
  fails in other part (uitour-zoom is not found)
* browser/components/uitour/test/browser_UITour3.js
  OK (does not touch the content of search bar)
* browser/components/uitour/test/browser_UITour_availableTargets.js
  fails because searchIcon is not found in availableTargets
* browser/components/uitour/test/browser_openSearchPanel.js
  fails because searchbar's textbox is not found
  the failure happens inside non-test code
  https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/browser/components/uitour/UITour.jsm#649
  (this test is disabled becasue it fails even if searchbar is shown, bug 1113038)

* browser/modules/test/browser/browser_UsageTelemetry_searchbar.js
  fails because searchbar's textbox is not found

* dom/events/test/browser_shortcutkey_modifier_conflicts_with_content_accesskey_modifier.js
  OK (does not touch search bar itself)

* layout/xul/test/browser_bug1163304.js
  fails because searchbar cannot get input

* toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields.js
  fails because searchbar's textbox is not found

locally I've added helper function to BrowserTestUtils.jsm, and it checks overflow.
failing tests except uitour tests are fixed locally.
Added BrowserTestUtils.showSearchBar function that shows search bar in nav bar, by setting browser.search.widget.inNavBar pref to true, and verifies it does not overflow from the nav bar.
In most files above, replaced the code that flips the pref (and get searchbar element) with BrowserTestUtils.showSearchBar call, except the following tests which I added comment around the assertion etc:

  * browser/components/customizableui/test/browser_901207_searchbar_in_panel.js
    this test touches the pref more than once and BrowserTestUtils.showSearchBar does not fit
  * browser/components/customizableui/test/browser_customization_context_menus.js
    this test is testing gCustomizeMode.addToPanel function which returns a
    promise that does not resolve because of the lack of animation event
    if overflows
    maybe this should be fixed in separate bug
  * browser/components/search/test/browser_google_behavior.js
    this test touches the pref more than once and BrowserTestUtils.showSearchBar does not fit

Then, to handle pref, BrowserTestUtils.showSearchBar receives SpecialPowers object as parameter,
is this okay?

I first tried clearing pref by registerCleanupFunction, but it conflicts with existing code in the tests, because of the execution order.
Cleanup functions are executed in the registered order, and some testcase registers cleanup function that touched searchbar, but if BrowserTestUtils.showSearchBar hides search bar before that, the code fails because the searchbar is not present.
Then, pref env is flushed after calling all cleanup functions, so I want to use SpecialPowers.pushPrefEnv there.
Attachment #8931835 - Attachment is obsolete: true
Flags: needinfo?(arai.unmht)
Attachment #8971155 - Flags: review?(paolo.mozmail)
Comment on attachment 8971155 [details] [diff] [review]
Add BrowserTestUtils.showSearchBar to show search bar in nav bar and verify it does not overflow.

It's great that these tests are getting some attention. Thanks!

We'd like to move away from using the preference to control the search bar positioning, this was just a shortcut I took to minimize the code changes while we were trying to land more important changes to the search bar for Firefox 57. This avoided duplicating the placement logic, because there was no obvious place for a shared helper back then.

Avoiding the preference is on file as bug 1396562, and your patch already addresses it by using a helper function, except for a few cases. We can leave the other bug open until those are also fixed.

The helpers to add and remove the search bar can now go in the recently added CustomizableUITestUtils module, since BrowserTestUtils should only contain generic browser-chrome helper function. I'd call the helpers addSearchBar and removeSearchBar, to be consistent with the functions they are using under the hood. They would perform the additional checks but basically use the same logic that is already applied indirectly through the preference listener:

https://dxr.mozilla.org/mozilla-central/rev/8b2c1fc3d6c348f053fe33a478fa3b1ddb5eb8a6/browser/components/customizableui/SearchWidgetTracker.jsm#64-71

(Note that you can inline the WIDGET_ID constant.)

I'd leave each test to do the cleanup by calling CustomizableUITestUtils.removeSearchBar, either as a cleanup function or just during the test. This way, you don't need to pass SpecialPowers to the module or find another way to import it.

I noticed a few cases where you wait for the next microtask. If these are still necessary after the other changes, add a comment as to why. Thanks!
Attachment #8971155 - Flags: review?(paolo.mozmail)
(In reply to Tooru Fujisawa [:arai] from comment #10)
> browser/components/customizableui/test/browser_customization_context_menus.js
>     this test is testing gCustomizeMode.addToPanel function which returns a
>     promise that does not resolve because of the lack of animation event
>     if overflows
>     maybe this should be fixed in separate bug

From the sound of it it's an actual bug, feel free to file it separately.
Also, CustomizableUITestUtils is already constructed as gCUITestUtils so you don't need to pass the reference to the window.
Comment on attachment 8971155 [details] [diff] [review]
Add BrowserTestUtils.showSearchBar to show search bar in nav bar and verify it does not overflow.

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

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ +1773,5 @@
> +
> +    // Check the existence of the icon in the searchbar, to make sure overflow
> +    // does not happen.
> +    let searchIcon = window.document.getAnonymousElementByAttribute(
> +      searchbar, "anonid", "searchbar-search-button");

Hm, can we use another way of checking whether "searchbar" is visible or not? This code seems to depend on the fact that the XBL binding is only applied to a visible element, so this may change when we convert the search bar to a Custom Element.
Thank you for reviewing :D

Added CustomizableUITestUtils#{addSearchBar,removeSearchBar}.

CustomizableUITestUtils#addSearchBar does a bit complicated thing now.
(instead of just waiting the next event tick without clear reason.  which turns out not to be the right thing to do here)

What we need to do is following:
  * Add the search bar into the nav bar
  * Check if the search bar doesn't overflow, which means either:
    * Successful case:
      the search bar will never be put into the overflow panel
    * Failure case:
      the search bar is put into the overflow panel

Then, in either case, the search bar is put into the nav bar first, and if overflow happens, the overflow is handled asynchronously here:
  https://searchfox.org/mozilla-central/rev/ce9ff94ffed34dc17ec0bfa406156d489eaa8ee1/browser/components/customizableui/CustomizableUI.jsm#4424-4472
until the end of the function, it's not clear which widget overflows even if the overflow event happens.

Also, we cannot wait for the overflow event since it's possible that nothing overflows.

Hence, CustomizableUITestUtils#addSearchBar does the following:
  * Add the search bar into the nav bar
  * Wait for the layout flush (window.promiseDocumentFlushed)
    what I expect is that, when the promise resolves, either the following happens:
    * the search bar is put into the nav bar and it gets stable
    * if overflow happens, CustomizableUITestUtils#onOverflow's synchronous part
      (before the first `await` there) is executed
    but I haven't yet confirmed that window.promiseDocumentFlushed is the right thing to wait for...
  * Check if `_lastOverflowCounter` property is incremented:
    * if it's zero, it means either:
      * no overflow happens
      * overflow happened and the overflow handling has finished
    * if it's zero, it means overflow happened and the overflow handling haven't yet finished
      in this case, wait until `_lastOverflowCounter` property becomes zero
  * Check if the overflow panel is the ancestor of the search bar
    * if so, throw error message which explains the remaining tests will also fail, and the reason why


In testcases:
  * In most case, add search bar by gCUITestUtils.addSearchBar() at first,
    and register cleanup function that removes search bar by gCUITestUtils.removeSearchBar()
    * If the testcase touches search bar in another cleanup function,
      call registerCleanupFunction again inside registerCleanupFunction to remove
      the search bar after all other functions finish
  * Added comment that describes the failure reason to
    * browser_901207_searchbar_in_panel.js
    * browser_customization_context_menus.js
Attachment #8971155 - Attachment is obsolete: true
Attachment #8972785 - Flags: review?(paolo.mozmail)
Comment on attachment 8972785 [details] [diff] [review]
Add CustomizableUITestUtils.prototype.{addSearchBar,removeSearchBar} to show search bar in nav bar and verify it does not overflow.

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

This looks great! I recommend some cleanup of the registerCleanupFunction calls, it will be a quick review once done.

::: browser/base/content/test/about/browser_aboutHome_search_searchbar.js
@@ +12,5 @@
>  
> +  let searchbar = await gCUITestUtils.addSearchBar();
> +  registerCleanupFunction(() => {
> +    gCUITestUtils.removeSearchBar();
> +  });

Since registerCleanupFunction runs after all the tests in the file, this should be rewritten so that if a new test is added, it starts with the expected environment regardless of whether the new test is added before or after this one.

Three alternatives:
1. Just add a call gCUITestUtils.removeSearchBar(); at the end of the test task.
2. Same, but factor it into a "function cleanup()" that is passed to registerCleanupFunction and also called at the end of the test task. We use this approach in a few places. In this case registerCleanupFunction is optional and just a precaution in case the test fails.
3. Add an "add_task(async function test_setup() {" before this test task that only contains the call to addSearchBar and registerCleanupFunction. This may be the best approach since it keeps the search bar visible for the entire test file, and registerCleanupFunction is part of the expected test flow. You can keep the getElementById call or store the reference in a gSearchInput variable.

::: browser/components/customizableui/test/CustomizableUITestUtils.jsm
@@ +1,1 @@
> +

Remove the empty line at the beginning.

@@ +108,5 @@
> +    //
> +    // We should first wait for the layout flush to make sure either the search
> +    // bar fits into the nav bar, or overflow event gets dispatched and the
> +    // overflow event handler is called.
> +    await this.window.promiseDocumentFlushed(() => {});

I also _believe_ this will return after the overflow event has been dispatched. At least, the intent is clear from the extensive code comments. Thanks for writing them!

@@ +114,5 @@
> +    // Check if the OverflowableToolbar is handling the overflow event.
> +    // _lastOverflowCounter property is incremented synchronously at the top
> +    // of the overflow event handler, and is set to 0 when it finishes.
> +    let navbar = this.window.document.getElementById(CustomizableUI.AREA_NAVBAR);
> +    if (navbar.overflowable._lastOverflowCounter > 0) {

We might as well skip the "if" check and always call waitForCondition.

@@ +116,5 @@
> +    // of the overflow event handler, and is set to 0 when it finishes.
> +    let navbar = this.window.document.getElementById(CustomizableUI.AREA_NAVBAR);
> +    if (navbar.overflowable._lastOverflowCounter > 0) {
> +      await TestUtils.waitForCondition(() => {
> +        return navbar.overflowable._lastOverflowCounter == 0;

Please use "===" here, with a comment, so if the variable is renamed or the approach changed, we'll see the tests fail instead of skipping the check silently.

::: browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js
@@ +15,5 @@
>  async function test_opensearch(shouldWork) {
> +  let searchBar = await gCUITestUtils.addSearchBar();
> +  registerCleanupFunction(() => {
> +    gCUITestUtils.removeSearchBar();
> +  });

Same here, with approach (3) recommended unless something else depends on the search bar not being visible.

::: browser/components/search/test/browser_healthreport.js
@@ +76,5 @@
>    }
>  
>    Services.obs.addObserver(observer, "browser-search-engine-modified");
> +  gCUITestUtils.addSearchBar()
> +  .then(function() {

nit: same line

::: browser/components/search/test/browser_oneOffHeader.js
@@ +57,5 @@
>  add_task(async function init() {
> +  searchbar = await gCUITestUtils.addSearchBar();
> +  registerCleanupFunction(() => {
> +    // Register again to run this after other cleanup functions in this test.
> +    registerCleanupFunction(() => {

Maybe you can just move the resetting of "searchbar._textbox.value" out of the other registerCleanupFunction call.

I also thought functions registered with registerCleanupFunction ran in reverse order... but maybe it's different between xpcshell and mochitests. If they are indeed different, can you file a bug in the "testing" component so we unify this at some point so they all run in reverse order?

::: browser/components/search/test/browser_searchbar_keyboard_navigation.js
@@ +26,5 @@
>  add_task(async function init() {
> +  searchbar = await gCUITestUtils.addSearchBar();
> +  registerCleanupFunction(() => {
> +    // Register again to run this after other cleanup functions in this test.
> +    registerCleanupFunction(() => {

Also here, the code in the rest of the file can be moved out of registerCleanupFunction. For the more complex case you can set up a try-finally block in the async function.

Most of the files below can also follow some of the approaches discussed.
Attachment #8972785 - Flags: review?(paolo.mozmail)
Added "test_setup" task which adds search bar, and registers cleanup function that removes search bar, in most testcases.

in testcases which registers another cleanup functions, moved those code into "cleanup" task. so that the task will be executed regardless of the previous task's result.

I'll check the execution order of cleanup functions shortly and file a bug if necessary.
Attachment #8972785 - Attachment is obsolete: true
Attachment #8973921 - Flags: review?(paolo.mozmail)
See Also: → 1459821
Comment on attachment 8973921 [details] [diff] [review]
Add CustomizableUITestUtils.prototype.{addSearchBar,removeSearchBar} to show search bar in nav bar and verify it does not overflow.

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

Superb, thank you!

::: browser/components/search/test/browser_oneOffHeader.js
@@ +144,5 @@
>    await synthesizeNativeMouseMove(searchbar);
>  });
> +
> +add_task(async function cleanup() {
> +    searchbar._textbox.value = "";

nit: indentation
Attachment #8973921 - Flags: review?(paolo.mozmail) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/882a5a8235be63dea073aa736ea57cd8f8eaaa71
Bug 1420574 - Add CustomizableUITestUtils.prototype.{addSearchBar,removeSearchBar} to show search bar in nav bar and verify it does not overflow. r=Paolo
https://hg.mozilla.org/mozilla-central/rev/882a5a8235be
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.