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

VERIFIED FIXED in Firefox 61

Status

defect
VERIFIED FIXED
a year ago
11 months ago

People

(Reporter: emilghitta, Assigned: yzen)

Tracking

(Blocks 1 bug)

Trunk
Firefox 62
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox60 unaffected, firefox61 verified, firefox62 verified)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

a year ago
Posted image 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.
(Assignee)

Comment 1

a year ago
Posted patch 1461977 alternative 1 (obsolete) — Splinter Review
This ensures that scrollTop value is in sync with scroll state when re-docking the toolbox.
Attachment #8980410 - Flags: review?(nchevobbe)
(Assignee)

Updated

a year ago
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
(Assignee)

Updated

a year ago
Attachment #8980410 - Attachment description: 1461977 part 1 → 1461977 alternative 1
(Assignee)

Comment 2

a year ago
Posted patch 1461977 alternative 2 (obsolete) — Splinter Review
Second alternative but this currently fails tree test 11 were focused node should move into view when arrowing down on last visible one.
Attachment #8980417 - Flags: review?(nchevobbe)
Yura, should I review both patches ?
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 ? )
Attachment #8980410 - Flags: review?(nchevobbe) → review+
(Assignee)

Updated

a year ago
Attachment #8980417 - Attachment is obsolete: true
Attachment #8980417 - Flags: review?(nchevobbe)
(Assignee)

Comment 5

a year ago
Posted patch 1461977 v2 (obsolete) — Splinter Review
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.
Attachment #8980410 - Attachment is obsolete: true
Attachment #8980753 - Flags: review?(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})

?
Attachment #8980753 - Flags: review?(nchevobbe) → review+
(Assignee)

Comment 7

a year ago
Carrying over r+
Attachment #8980753 - Attachment is obsolete: true
Attachment #8981218 - Flags: review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Comment 8

a year ago
Hi Emil, when this lands on nightly could you verify that the bug is fixed for you along with the bug 
1443155
?
Flags: needinfo?(emil.ghitta)

Comment 9

a year ago
Pushed by apavel@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b8695f808fb0
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
(Assignee)

Comment 11

a year ago
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
Attachment #8981218 - Flags: approval-mozilla-beta?
Comment on attachment 8981218 [details] [diff] [review]
1461977 patch v3

a11y explorer fix, approved for 61.0b10.
Attachment #8981218 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
(Reporter)

Comment 14

a year ago
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.
Status: RESOLVED → VERIFIED
(Reporter)

Comment 15

a year ago
This issue is verified fixed using Firefox 61.0b10 (BuildId:20180530184300) on Windows 10 64bit, macOS 10.12.6 and Ubuntu 16.04 64bit.
Flags: qe-verify+
Flags: needinfo?(emil.ghitta)

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.