Steve Melia Assignee
	Description • 8 years ago

Bug 1259812 reimplements the breadcrumbs using XHTML instead of XUL; but will not cover RTL for the horizontal scrolling.

	Steve Melia Assignee
	Comment 1 • 8 years ago

See also Bug 1267802

	Steve Melia Assignee
	Comment 2 • 8 years ago

I'll have a crack at this if no-one else gets there first!

	Patrick Brosset <:pbro>
	Comment 3 • 8 years ago

Inspector bug triage (filter on CLIMBING SHOES).

	Steve Melia Assignee
	Comment 4 • 8 years ago

I used an extension "Force RTL" to manually test the attached patch.

I feel breadcrumb scrolling is reasonably complex and could do with some tests; but I cannot find any clear examples (grepping devtools/client) of mochitest checking RTL behaviour and I suppose I would be testing visibility anyway - i'm not sure how this would work for different screen sizes, or if it is possible to enforce particular dimensions for a test.

I was thinking a unit test style might be more appropriate here; just for the two methods that find the first/last element, stubbing the element bounds. If you agree, are there examples of this anywhere? Would it be unexpected to find a test like this in a mochitest?

	Steve Melia Assignee
	Comment 5 • 8 years ago

Manual test case for issue identified in Bug 1259812 Comment 63 - this is fixed with this patch due to including the offset of the overflow arrows before bounds checking.

	Patrick Brosset <:pbro>
	Comment 6 • 8 years ago

Sorry for the delay Steve, last week we had a Mozilla-wide event that didn't leave any time for reviews. And, unfortunately, this week I find myself really sick and won't be able to get to the review either.
I suggest asking :jdescottes for info and reviews here.

	Julian Descottes [:jdescottes]
	Comment 7 • 8 years ago

(In reply to Steve Melia from comment #4)
> 
> I feel breadcrumb scrolling is reasonably complex and could do with some
> tests; but I cannot find any clear examples (grepping devtools/client) of
> mochitest checking RTL behaviour and I suppose I would be testing visibility
> anyway - i'm not sure how this would work for different screen sizes, or if
> it is possible to enforce particular dimensions for a test.

It is possible to resize the test window, and you can use preferences to set the toolbox dimensions and UI direction (ie RTL).

To resize the window, you can use window::resizeTo. Have a look at browser_inspector_pane-toggle-04.js for an example (https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/test/browser_inspector_pane-toggle-04.js).

Updating preferences can be done using the pushPref() helper defined in shared-head.js. The helper also takes care of restoring the modified preferences after the test. Typically, you want to update the preferences before adding the test tab and opening the devtools.

To set the dimensions of the toolbox, you can set the preference "devtools.toolsidebar-width.inspector". 
This preference will set the width of the sidebar displayed on the right side of the inspector: 

> yield pushPref("devtools.toolsidebar-width.inspector", value);

To set the UI direction to RTL you can use:

> yield pushPref("intl.uidirection.en-US", "rtl");

It is not used anywhere else in devtools, but I found one test in the firefox codebase using a similar approach to test RTL: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/tests/chrome/test_bug437844.xul. Now since this is not extensively used, some of our test helpers might not work as expected. So I would focus on writing a non-RTL test first before trying the RTL version.

With all this it "should" be possible to recreate the conditions you need for this test.  
Then as you said, you will have to test for visibility here, but I think that could still be a valid test to write. 

> I was thinking a unit test style might be more appropriate here; just for
> the two methods that find the first/last element, stubbing the element
> bounds. If you agree, are there examples of this anywhere? Would it be
> unexpected to find a test like this in a mochitest?

I think it's fine to stub parts of the implementation, if it is documented _and_ if we still need to actually test the UI. If the test is not relying on any UI, then we should consider using unit tests (for instance see devtools/client/animationinspector/test/unit/test_findOptimalTimeInterval.js). In this particular case, I still think it would be nice to have an integration test verifying the scroll. If the only thing to stub are the container's dimensions, it can also be achieved using preferences as explained above.

	Steve Melia Assignee
	Comment 9 • 8 years ago

Hi guys so sorry this has taken so long, I have been a little busy. jdescottes may I ask for an r?

Patch has been linted and subset of tests run locally; but needs a try. (I don't have perms) there are two problems I need to draw your attention to:

- I could not replicate under automation the manual test (test-overflow.html) prior to implementing RTL. So the automated test doesn't cover this issue although I confirmed manually that it is resolved.

- Because I use element.scrollIntoView in the breadcrumbs, I run into Bug 1172171 and use waitForTime to work around this. The test takes 43 seconds locally, I believe the default limit if 45s - there is an obvious concern that it will either take too long or I will end up creating an intermittent if there's a test slave with a slow scroll.

	Julian Descottes [:jdescottes]
	Comment 10 • 8 years ago

(In reply to Steve Melia from comment #9)
> Created attachment 8774069 [details] [diff] [review]
> 1275399.patch
> 
> Hi guys so sorry this has taken so long, I have been a little busy.
> jdescottes may I ask for an r?
> 

Thanks for your patch Steve! I'll be on away starting on Wednesday but I'll do my best to do a first review before leaving (and will forward if I don't manage).

> Patch has been linted and subset of tests run locally; but needs a try. (I
> don't have perms) 

If you want, you can ask for a commit access level 1 :) This way you can push to try by yourself. You can use bug 1184334 as a reference if you want.

I pushed the changeset to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b80be62fff63d16168937ac37de545ef12b0f859

As you can see, the new test is failing on Linux & OSX. I can reproduce this locally too when running the test: 
- in the LTR part, when going back from FOUR to FIVE clicking on the "end" button, the breadcrumbs simply don't scroll. I'll try to dig more to see if I can find why (not sure you have the resources to test osx/linux ?)

> - I could not replicate under automation the manual test
> (test-overflow.html) prior to implementing RTL. So the automated test
> doesn't cover this issue although I confirmed manually that it is resolved.
> 
> - Because I use element.scrollIntoView in the breadcrumbs, I run into Bug
> 1172171 and use waitForTime to work around this. The test takes 43 seconds
> locally, I believe the default limit if 45s - there is an obvious concern
> that it will either take too long or I will end up creating an intermittent
> if there's a test slave with a slow scroll.

Regarding the test duration, you are completely correct: the timeout threshold is 45 seconds. Looking at the try push, the new test takes ~35-45 seconds on Linux/Linux debug already, so no doubt it will intermittently timeout. 
One option would be to add "requestLongerTimeout(2)" at the beginning of the test, this way the test can last 90 seconds.

Or we could disable the smooth scrolling in the breadcrumbs when running the tests? This means we need to make this configurable from the outside but that could make the test much faster & reliable.

	Julian Descottes [:jdescottes]
	Comment 11 • 8 years ago

Comment on attachment 8774069 [details] [diff] [review]
1275399.patch

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

Before clearing the review flag, we should address the two main points I discussed in my previous comment:
- failing test and behavior issues on OSX/linux: it looks like there are rounding issues when using the smooth scrollIntoView on OSX, which means that findFirst/LastInvisibleElement might forever return the same element

Using 1 px extra margin seems to fix the issue for me : 


>  /**
>   * Get the first (i.e. furthest left for LTR)
>   * non or partly visible element in the scroll box
>   */
>  getFirstInvisibleElement: function () {
>    let elementsList = Array.from(this.inner.childNodes).reverse();
>    let predicate = (left, right, elementLeft, elementRight) => this.isRtl()
>      ? elementRight > (right + 1) && elementLeft > (left + 1)
>      : elementLeft < (left - 1) && elementRight < (right - 1);
>    return this.findFirstWithBounds(elementsList, predicate);
>  },
>  
>  /**
>   * Get the last (i.e. furthest right for LTR)
>   * non or partly visible element in the scroll box
>   */
>  getLastInvisibleElement: function () {
>    let predicate = (left, right, elementLeft, elementRight) => this.isRtl()
>      ? elementLeft < (left - 1) && elementRight < (right - 1)
>      : elementLeft > (left + 1) && elementRight > (right + 1);
>    return this.findFirstWithBounds(this.inner.childNodes, predicate);
>  },

There might be a better way to handle this but I didn't investigate much more.

- try to make the test run faster by disabling the smooth scrolling

	Steve Melia Assignee
	Comment 12 • 8 years ago

1) I could not replicate the test failure locally (on Linux) - i've added a SCROLL_MARGIN const for this. I am a bit concerned it's a timing issue. If so; Bug 1274838 would at least offer a workaround in that the user could click the arrow again. (At the moment an interrupted smooth scroll seems to prevent subsequent smooth scrolls from being effected)

2) the scrolling is now instant (for automation only) and the test is much quicker; mostly startup/cleanup overhead.

Attached patch pending try push (see Bug 1290728) :)

	Steve Melia Assignee
	Comment 13 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=65acd0b991f8

	Steve Melia Assignee
	Comment 14 • 8 years ago

Comment on attachment 8776380 [details] [diff] [review]
1275399-instant-scroll.patch

First try, great! Test was chunked into dt4 (found by searching the chunk logs, I guess you just get a feel for where they will be..?) and passed in 5,006ms.

	Julian Descottes [:jdescottes]
	Comment 15 • 8 years ago

Comment on attachment 8776380 [details] [diff] [review]
1275399-instant-scroll.patch

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

Thanks for the patch Steve, and congratulations on your first try push :)

Mostly nits remaining, plus one thing I missed regarding using new chrome-privileged APIs.
Let's do one last review round after you address and we should be good to go.

::: devtools/client/inspector/breadcrumbs.js
@@ +47,5 @@
>  
>  ArrowScrollBox.prototype = {
>  
> +  // Scroll behaviour, exposed for testing
> +  scrollBehaviour: "smooth",

nit: For consistency with the scrollIntoView API, should be scrollBehavior and not scrollBehaviour

@@ +57,5 @@
>    init: function () {
>      this.constructHtml();
> +    let chromeReg = Cc["@mozilla.org/chrome/chrome-registry;1"]
> +      .getService(Ci.nsIXULChromeRegistry);
> +    this.isRtl = () => chromeReg.isLocaleRTL("global");

We are trying to remove as much chrome privileged code as we can. In order to know if we are in RTL here, we could do :

> this.isRtl = () => this.win.getComputedStyle(this.container).direction === "rtl";

Let's also take the apportunity to move isRtl  to the prototype of ArrowScrollBox instead of defining it here in the init method.

@@ +83,5 @@
>  
>    /**
> +   * Scroll to the specified element using the current scroll behaviour
> +   * @param {element} element element to scroll
> +   * @param {string} block desired alignment of element after scrolling

nit: element -> Element 
     string -> String

@@ +94,3 @@
>     * Call the given function once; then continuously
>     * while the mouse button is held
> +   * @param {function} repeatFn the function to repeat while the button is held

nit: function -> Function

@@ +224,5 @@
> +   * not also span the entire container.
> +   * @param {Number} left the left scroll point of the container
> +   * @param {Number} right the right edge of the container
> +   * @param {Number} the left edge of the element
> +   * @param {Number} the right edge of the element

nit: missing parameter names elementLeft & elementRight

@@ +237,5 @@
> +   * not also span the entire container.
> +   * @param {Number} left the left scroll point of the container
> +   * @param {Number} right the right edge of the container
> +   * @param {Number} the left edge of the element
> +   * @param {Number} the right edge of the element

nit: missing parameter names elementLeft & elementRight

@@ +269,5 @@
> +
> +  /**
> +   * Find the first element that matches the given predicate, called with bounds
> +   * information
> +   * @param {iterator} iterator an ordered list of elements

I think this is actually an Array (Iterable) not an Iterator, let's document it as Array, will be easier.

@@ +270,5 @@
> +  /**
> +   * Find the first element that matches the given predicate, called with bounds
> +   * information
> +   * @param {iterator} iterator an ordered list of elements
> +   * @param {function} predicate a function to be called with bounds

nit function -> Function

::: devtools/client/inspector/test/browser_inspector_breadcrumbs_visibility.js
@@ +1,5 @@
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +"use strict";
> +

Please add a short comment here to describe what this test is about.

	Julian Descottes [:jdescottes]
	Comment 16 • 8 years ago

One last thing: can you amend the commit message. A typical commit message looks like:

> Bug 123456 - Change this thing to work better by doing something. r=reviewers

	Steve Melia Assignee
	Comment 17 • 8 years ago

Thanks Julian & see attached.

	Julian Descottes [:jdescottes]
	Comment 18 • 8 years ago

Comment on attachment 8779076 [details] [diff] [review]
1275399-nits.patch

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

Looks good, thanks Steve! 

Submitted the new patch to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=92ca3409f6d2
Let's wait for the try results and add checkin-needed if the tests are passing. Thanks for working on this!

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 19 • 8 years ago

This is failing to cleanly apply, can we get a patch rebased to the tip of the repository? 
$ hg import -e https://bugzilla.mozilla.org/attachment.cgi?id=8779076
applying https://bugzilla.mozilla.org/attachment.cgi?id=8779076
patching file devtools/client/inspector/breadcrumbs.js
Hunk #6 FAILED at 552
1 out of 7 hunks FAILED -- saving rejects to file devtools/client/inspector/breadcrumbs.js.rej
abort: patch failed to apply

	Steve Melia Assignee
	Comment 20 • 8 years ago

no problem, see attached.

	Pulsebot
	Comment 21 • 8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/28ded847d319
Change inspector breadcrumbs to add RTL functionality. r=jdescottes

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 22 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/28ded847d319