Closed Bug 1519953 Opened 10 months ago Closed 9 months ago

Remove boxObject x/y/width/height callers

Categories

(Core :: XUL, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: enndeakin, Assigned: enndeakin)

References

Details

Attachments

(1 file, 1 obsolete file)

Instead use getBoundingClientRect which is mostly equivalent.

Priority: -- → P3
Attachment #9036406 - Flags: review?(gijskruitbosch+bugs)

Does element.boxObject (and/or its property getters) currently always flush layout if it's dirty?

Flags: needinfo?(enndeakin)

Yes, they always flush layout (using FlushType::Layout).

Flags: needinfo?(enndeakin)
Comment on attachment 9036406 [details] [diff] [review]
Remove calls to boxObject x/y/width/height

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

Mostly these are fine, but given it's a big patch, there's still a bunch of things that don't look right, see comments below:

::: accessible/tests/mochitest/hittest/test_zoom_tree.xul
@@ +44,5 @@
>        var treeAcc = getAccessible(tree, [nsIAccessibleTable]);
>        var cellAcc = treeAcc.getCellAt(1, 0);
>        var rowAcc = cellAcc.parent;
>  
> +      var cssX = rect.x + treeRect.left;

Out of xuriosity, why change some of these from `x` to `left` (e.g. here) and not others (e.g. menu.xml)?

::: browser/base/content/tabbrowser.xml
@@ +995,5 @@
>          <parameter name="isLink"/>
>          <body><![CDATA[
>            let tab = event.target.localName == "tab" ? event.target : null;
>            if (tab && isLink) {
> +            let width = tab.getBoundingClientRect().width;

Nit: might as well `let {width} = tab.getBoundingClientRect();`

::: browser/base/content/test/general/browser_tab_dragdrop2_frame1.xul
@@ +67,5 @@
>  function popupShown(event)
>  {
>    var panel = event.target;
>    if (waitSteps > 0 && navigator.platform.includes("Linux") &&
> +      panel.screenY == 210) {

Off-hand, this change doesn't look right? When do elements have a `screenY` property? In particular, if I run this in the browser console:

```
x = (e) => console.log(e.target.screenY); PanelUI.panel.addEventListener("popupshown", x);
```

it logs `undefined` when opening the hamburger panel.

::: browser/components/places/content/menu.xml
@@ +86,5 @@
>              let scrollbox = this._scrollBox;
> +
> +            let thisY = this.getBoundingClientRect().y;
> +            let scrollboxY = scrollbox.getBoundingClientRect().y;
> +            let innerScrollboxY = scrollbox._scrollbox.getBoundingClientRect().y;

Not a fan of reaching into the scrollbox binding for _underscored properties like this; can we file a follow-up to address that (I see we already do it in other places in this file)? I'm worried we will not notice this when porting the scrollbox binding given how little this special places menu binding is really used...

@@ +91,5 @@
> +
> +            let eventY = aEvent.layerY + (scrollboxY - thisY);
> +            let scrollboxOffset = innerScrollboxY - (scrollboxY - thisY);
> +            let eltY = elt.getBoundingClientRect().y - scrollboxOffset;
> +            let eltHeight = elt.getBoundingClientRect().height;

Nit:

let {y: eltY, height: eltHeight} = elt.getBoundingClientRect();
eltY -= scrollboxOffset;

@@ +442,5 @@
>          let newMarginTop = 0;
>          if (scrollDir == 0) {
>            let elt = this.firstElementChild;
>            while (elt && event.screenY > elt.screenY +
> +                                        elt.scrollRect.height / 2)

Looks like you want `s/elt.//` here.

@@ +452,3 @@
>  
>          // Set the new marginTop based on arrowscrollbox.
> +        newMarginTop += scrollRect.y - this._scrollBox.getBoundingClientRect().y;

This doesn't make sense. `scrollRect == this._scrollBox.getBoundingClientRect()`, and there are no changes to the DOM inbetween when we get `scrollRect` and this line (and in any case, IIRC `getBoundingClientRect()` returns a live rect...). So effectively this line is `foo += bar - bar` which doesn't do anything.

Before this patch, we check the difference between this._scrollBox.boxObject and this._scrollBox.scrollBoxObject.

::: browser/components/preferences/in-content/findInPage.js
@@ +339,5 @@
>          let result = this.highlightMatches([node], [node.length], node.textContent.toLowerCase(), searchPhrase);
>          matchesFound = matchesFound || result;
>        }
>  
> +      // Collecting data from rect / label / description

We don't really use rects here, do we?

@@ +344,4 @@
>        let nodeSizes = [];
>        let allNodeText = "";
>        let runningSize = 0;
> +      let accessKeyTextNodes = this.textNodeDescendants(nodeObject);

This doesn't look right; the helper in question (which isn't being changed) just iterates over descendants. Then we run it again, passing the same arg, if `nodeObject` is a `<label>` or `<description>`. What does the box object thing do? Nothing?

::: layout/base/crashtests/526378-1.xul
@@ +12,5 @@
>    
>    x.appendChild(document.createTextNode("A"));
>    x.appendChild(document.createTextNode("\u202B" + "C"));
>  
> +  document.documentElement.getBoundingClientRect(); // flush layout

Does just fetching it do so, or fetching any of its props? I'd add in the `.height` fetch here.

::: layout/xul/test/test_bug393970.xul
@@ +76,5 @@
>      function checkPositions(variant) {
>        for (var col = 1; col <= 3; col++) {
> +        is($('cell1' + col).getBoundingClientRect().x, $('cell2' + col).getBoundingClientRect().x,
> +           "Cells (1," + col + ") and (2," + col + ") line up horizontally (with " + variant + ")");
> +        is($('cell2' + col).getBoundingClientRect().x, $('cell3' + col).getBoundingClientRect().x,

Do the boxObject getters round? If so, we might need to do the same here...

::: toolkit/content/widgets/popup.xml
@@ -159,5 @@
>            var width = 0;
>            for (var menuitem = this.firstElementChild; menuitem; menuitem = menuitem.nextElementSibling) {
>              if (menuitem.localName == "menuitem" && menuitem.hasAttribute("acceltext")) {
>                var accel = document.getAnonymousElementByAttribute(menuitem, "anonid", "accel");
> -              if (accel && accel.boxObject) {

Can we optimize anything here, like checking if the menuitem has a hidden attribute? What does the boxObject check do otherwise?

::: toolkit/content/widgets/tree.js
@@ +275,5 @@
>        return "" + (val == "0" ? 0 : parseInt(val));
>      }
>  
>      get _previousVisibleColumn() {
>        var sib = this.boxObject.previousSibling;

Why are we leaving this? What's the difference between `this.previousSibing` and `this.boxObject.previousSibling` ?

::: toolkit/modules/PopupNotifications.jsm
@@ +988,5 @@
>      this._refreshPanel(notificationsToShow);
>  
> +    function isNullOrHidden(elem) {
> +      if (!elem) {
> +        return false;

Previously, the condition passed if the element was falsy, so this should return true, I think?
Attachment #9036406 - Flags: review?(gijskruitbosch+bugs) → review-

Out of xuriosity, why change some of these from x to left (e.g. here) and not others (e.g. menu.xml)?

I will change to at least always use left/top instead of x/y.

Off-hand, this change doesn't look right? When do elements have a screenY property? In particular, if I run this in the browser console:

Sorry, this is added by bug 1519952.

::: browser/components/preferences/in-content/findInPage.js

This doesn't look right; the helper in question (which isn't being changed) just iterates over descendants. Then we run it again, passing the same arg, if nodeObject is a <label> or <description>. What does the box object thing do? Nothing?

It looks like I will need to fix this some other way. It's relying on displayed order given by the box objects.

Does just fetching it do so, or fetching any of its props? I'd add in the .height fetch here.

The flush occurs at the beginning of getBoundingClientRect. I will change it to clientHeight since that involves less work.

Can we optimize anything here, like checking if the menuitem has a hidden attribute? What does the boxObject check do otherwise?

I don't think the conditional check for the boxObject is useful.

Why are we leaving this? What's the difference between this.previousSibing and this.boxObject.previousSibling ?

This reference is removed in bug 1519960.

The findinpage part I will fix in a separate bug.

Attachment #9036406 - Attachment is obsolete: true
Attachment #9046025 - Flags: review?(gijskruitbosch+bugs)

Apologies for the delay, will try to get to this tomorrow morning.

Comment on attachment 9046025 [details] [diff] [review]
Remove calls to boxObject x/y/width/height

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

::: layout/xul/crashtests/399013.xul
@@ +1,1 @@
> +<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

What's this diff; line endings or something?

::: toolkit/content/widgets/tree.js
@@ +343,1 @@
>            return sib;

Nit: please could you add braces because the if condition is multiline now.
Attachment #9046025 - Flags: review?(gijskruitbosch+bugs) → review+

+<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

What's this diff; line endings or something?

The only change of note is the boxObject part at the bottom. I don't know what is going on with the first block of lines.

Pushed by neil@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa8dea3c0f43
replace calls to retrieve boxobject position and size with getBoundingClientRect, r=gijs
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Depends on: 1533693
You need to log in before you can comment on or make changes to this bug.