1461977 - Several elements from the "Properties" panel are not displayed after changing the Developer Tools panel position

Status: VERIFIED FIXED

(DevTools :: Accessibility Tools, defect)

Description

[Emil Ghitta, QA [:emilghitta]]

Attachment: issue.gif

[Affected versions]:
Firefox 62.0a1 (BuildId:20180515220059).
Firefox 61.0b5 (BuildId:20180514150347).

[Affected platforms]:
Windows 10 64bit.
macOS 10.12
Ubuntu 16.04 64bit

[Steps to reproduce]:
1. Launch Firefox.
2. Access any website.
3. Enable the Accessibility features.
4. Expand several elements from the "Properties" panel.
5. Dock the "Developer Tools" panel to side.

[Expected result]:
The panel displays the same information as before changing the position.

[Actual result]:
Some information is not displayed under the "Properties panel".

[Regression range]:
I don't think that this is a regression.

[Additional information]:
For further information regarding this issue please observe the attached screencast.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1461977 alternative 1

This ensures that scrollTop value is in sync with scroll state when re-docking the toolbox.

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1461977 alternative 2

Second alternative but this currently fails tree test 11 were focused node should move into view when arrowing down on last visible one.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Yura, should I review both patches ?

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8980410 [details] [diff] [review]
1461977 alternative 1

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

This change looks good to me, and I prefer it to alternative 2 because it's more explicit what it is doing.
We also need a test to make sure the bug is fixed and that we don't regress it.

::: devtools/client/shared/components/VirtualizedTree.js
@@ +345,2 @@
>     */
> +  _updateGeometry() {

nit: not sure about the name here, it isn't obvious what it does unless you look into it (maybe updateHeightAndScrollPosition ? )

Comment 5

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1461977 v2

This fixes the issue with scrollTop being re-set and instead syncs it up with the scroll state that we want to preserve on-resize.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8980753 [details] [diff] [review]
1461977 v2

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

I tested the patch and this seems good to me.
Again, it would be nice if there were a test to ensure we do fix the bug and we don't regress :)

::: devtools/client/shared/components/VirtualizedTree.js
@@ +459,5 @@
> +    // When tree size changes without direct user action, scroll top cat get re-set to 0
> +    // (for example, when tree height changes via CSS rule change). We need to ensure that
> +    // the tree's scrollTop is in sync with the scroll state.
> +    if (this.state.scroll !== this.refs.tree.scrollTop) {
> +      this.refs.tree.scrollTo(0, this.state.scroll);

nit: would you mind doing it the option way so it's easier to get what's done directly: 
scrollTo({left: 0, top: this.state.scroll})

?

Comment 7

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1461977 patch v3

Carrying over r+

Comment 8

[Yura Zenevich [:yzen] (please use "needinfo")]

Hi Emil, when this lands on nightly could you verify that the bug is fixed for you along with the bug 
1443155
?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8695f808fb0
sync tree's scrollTop with scroll state when tree scrollTop value changes outside of user scroll. r=nchevobbe

Comment 10

[Stefan Hindli [:stefan_hindli]]

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

Comment 11

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8981218 [details] [diff] [review]
1461977 patch v3

[Feature/Bug causing the regression]: Not a regression, one of the high priority bugs for a11y inspector panel feature
[User impact if declined]: If declined, as described in the bug (+ related bug) there are times when properties sidebar gets rendered as half empty and looks broken.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Waiting for confirmation.
[Needs manual test from QE? If yes, steps to reproduce]: Same steps as in the description.
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: No
[Why is the change risky/not risky?]: All the change does is scrolling the properties in sidebar to an original position.
[String changes made/needed]: None

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8981218 [details] [diff] [review]
1461977 patch v3

a11y explorer fix, approved for 61.0b10.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5e583b4b75c6

Comment 14

[Emil Ghitta, QA [:emilghitta]]

This issue is verified fixed using Firefox 62.0a1 (BuildID:20180529220039) on Windows 10 64bit, macOS 10.10.5 and Ubuntu 16.04 64bit.

Leaving the ni on myself as a reminder to verify this on 61.0b10 as well.

Comment 15

[Emil Ghitta, QA [:emilghitta]]

This issue is verified fixed using Firefox 61.0b10 (BuildId:20180530184300) on Windows 10 64bit, macOS 10.12.6 and Ubuntu 16.04 64bit.