Closed Bug 1453010 Opened 6 years ago Closed 6 years ago

Clicking the "target" icon doesn't lock the highlighter on the element

Categories

(DevTools :: Inspector: Animations, defect, P1)

Product:
DevTools
Component:
Inspector: Animations
Version:
61 Branch
Platform:
All
Unspecified
Type:
defect

Tracking

(firefox-esr52 unaffected, firefox60 unaffected, firefox61 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox60 --- unaffected
firefox61 --- verified

People

(Reporter: gpalko, Assigned: daisuke)

References

(Blocks 2 open bugs)

Details

(Keywords: regression)

Attachments

(4 files)

Bug 1453010 - Part 1: Lock highlighted element by clicking on the inspect icon in AnimationTarget component.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1453010 - Part 2: Change the title of inspect icon.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1453010 - Part 3: Select a node by clicking on dom node element in AnimationTargetCompositor.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1453010 - Part 4: Add test for locking highlighting.
59 bytes, text/x-review-board-request
gl
: review+
Details 
[Environment:]
Windows 8.1, Windows 10, Mac OSX 10.13.2
Nightly 61.0a1 2018-04-04

[Steps:]
1. Open Firefox Nightly and load https://rawgit.com/dadaa/3b73f847427025b51ba1ab7333013d0c/raw/77f3f0bb884875a179c3407f73bf8a8dd54751c9/doc_simple_animation.html.
2. Press F12.
3. Select Inspector.
4. Select Animation tab.
5. Click the "target" button near an element locator(e.g. div.ball.multi) and move the mouse cursor away from the button

[Actual Result:]
The element is not highlighted.

[Expected Result:]
5. Animation elements should be highlighted if their "target" button was clicked
Comment on attachment 8966900 [details]
Bug 1453010 - Part 1: Lock highlighted element by clicking on the inspect icon in AnimationTarget component.

https://reviewboard.mozilla.org/r/235558/#review241296


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/animation/components/AnimationTarget.js:83
(Diff revision 1)
>      const {
>        onHideBoxModelHighlighter,
>        onShowBoxModelHighlighterForNode,
> +      highlightingNodeActorID,
> +      lockHighlighting,
>        setSelectedNode,

Error: 'setselectednode' is assigned a value but never used. [eslint: no-unused-vars]
Comment on attachment 8966901 [details]
Bug 1453010 - Part 2: Change the title of inspect icon.

https://reviewboard.mozilla.org/r/235560/#review241298


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/animation/components/AnimationTarget.js:85
(Diff revision 1)
>      const {
>        onHideBoxModelHighlighter,
>        onShowBoxModelHighlighterForNode,
>        highlightingNodeActorID,
>        lockHighlighting,
>        setSelectedNode,

Error: 'setselectednode' is assigned a value but never used. [eslint: no-unused-vars]
These patches depend on following PR:
https://github.com/devtools-html/devtools-core/pull/1028
Depends on: 1452566
Assignee: nobody → dakatsuka
Priority: -- → P1
Blocks: 1453614
Friendly review ping!
I am wondering if this should actually be a feature. The target icon is actually overused in a number of different places and for different reasons. In the rules, the target icon acts as a selector highlighter toggle and in the grid panel it acts as a select element and a brief box model highlighter only on hover. This bug would make it behave like the former. 

If we wanted to make the changes so it is highlighted briefly, we can just do what is seen here and simplify the entire solution. https://searchfox.org/mozilla-central/source/devtools/client/inspector/grids/components/GridItem.js#117
Disregard what I said, I see it is a current feature in the old animation inspector.
Comment on attachment 8966900 [details]
Bug 1453010 - Part 1: Lock highlighted element by clicking on the inspect icon in AnimationTarget component.

https://reviewboard.mozilla.org/r/235558/#review243730

::: devtools/client/inspector/animation/actions/animations.js:48
(Diff revision 2)
>        elementPickerEnabled,
>      };
>    },
>  
>    /**
> +   * Update the highlighting node.

s/highlighting/highlighted

::: devtools/client/inspector/animation/actions/animations.js:50
(Diff revision 2)
>    },
>  
>    /**
> +   * Update the highlighting node.
> +   */
> +  updateHighlightingNode(nodeFront) {

updateHighlightedNode

::: devtools/client/inspector/animation/actions/index.js:20
(Diff revision 2)
>    "UPDATE_DETAIL_VISIBILITY",
>  
>    // Update state of the picker enabled.
>    "UPDATE_ELEMENT_PICKER_ENABLED",
>  
> +  // Update highlighting node.

s/highlighting/highlighted

::: devtools/client/inspector/animation/actions/index.js:21
(Diff revision 2)
>  
>    // Update state of the picker enabled.
>    "UPDATE_ELEMENT_PICKER_ENABLED",
>  
> +  // Update highlighting node.
> +  "UPDATE_HIGHLIGHTING_NODE",

s/HIGHLIGHTING/HIGHLIGHTED

::: devtools/client/inspector/animation/animation.js:41
(Diff revision 2)
>        this.addAnimationsCurrentTimeListener.bind(this);
>      this.getAnimatedPropertyMap = this.getAnimatedPropertyMap.bind(this);
>      this.getAnimationsCurrentTime = this.getAnimationsCurrentTime.bind(this);
>      this.getComputedStyle = this.getComputedStyle.bind(this);
>      this.getNodeFromActor = this.getNodeFromActor.bind(this);
> +    this.lockHighlighting = this.lockHighlighting.bind(this);

s/lockHighlighting/setHighlightedNode

::: devtools/client/inspector/animation/animation.js:175
(Diff revision 2)
>        this.simulatedAnimationForKeyframesProgressBar = null;
>      }
>  
>      this.stopAnimationsCurrentTimeTimer();
>  
> +    if (this.highlighter) {

I think we can remove this and just call  onHideBoxModelHighlighter

::: devtools/client/inspector/animation/animation.js:269
(Diff revision 2)
> +   *
> +   * @param {NodeFront} nodeFront
> +   */
> +  async lockHighlighting(nodeFront) {
> +    if (!this.highlighter) {
> +      this.highlighter = await this.inspector.toolbox.highlighterUtils

This should be calling onShowBoxModelHighlighterForNode

::: devtools/client/inspector/animation/animation.js:273
(Diff revision 2)
> +    if (!this.highlighter) {
> +      this.highlighter = await this.inspector.toolbox.highlighterUtils
> +                                   .getHighlighterByType("BoxModelHighlighter");
> +    }
> +
> +    await this.highlighter.hide();

onHideBoxModelHighlighter

::: devtools/client/inspector/animation/components/AnimationTarget.js:99
(Diff revision 2)
> +    const isHighlighting = nodeFront.actorID === highlightingNodeActorID;
> +
>      return dom.div(
>        {
> -        className: "animation-target"
> +        className: "animation-target" +
> +                   (isHighlighting ? " highlighting" : ""),

s/isHighlighting/isHighlighted

::: devtools/client/inspector/animation/reducers/animations.js:22
(Diff revision 2)
>  
>  const INITIAL_STATE = {
>    animations: [],
>    detailVisibility: false,
>    elementPickerEnabled: false,
> +  highlightingNodeActorID: null,

s/highlightingNodeActorID/highlightedNode

::: devtools/client/inspector/animation/reducers/animations.js:62
(Diff revision 2)
>      return Object.assign({}, state, {
>        elementPickerEnabled
>      });
>    },
>  
> +  [UPDATE_HIGHLIGHTING_NODE](state, { highlightingNodeActorID }) {

UPDATE_HIGHLIGHTED_NODE
Attachment #8966900 - Flags: review?(gl) → review+
Comment on attachment 8966901 [details]
Bug 1453010 - Part 2: Change the title of inspect icon.

https://reviewboard.mozilla.org/r/235560/#review243742
Attachment #8966901 - Flags: review?(gl) → review+
Comment on attachment 8966902 [details]
Bug 1453010 - Part 3: Select a node by clicking on dom node element in AnimationTargetCompositor.

https://reviewboard.mozilla.org/r/235562/#review243744

::: devtools/client/inspector/animation/components/AnimationTarget.js:111
(Diff revision 2)
>          {
>            defaultRep: ElementNode,
>            mode: MODE.TINY,
>            inspectIconTitle: getInspectorStr("inspector.nodePreview.highlightNodeLabel"),
>            object: translateNodeFrontToGrip(nodeFront),
> +          onDOMNodeClick: () => setSelectedNode(nodeFront, { reason: "animation-panel" }),

Consider also scrolling into view like https://searchfox.org/mozilla-central/source/devtools/client/inspector/grids/components/GridItem.js#86
Attachment #8966902 - Flags: review?(gl) → review+
Comment on attachment 8966902 [details]
Bug 1453010 - Part 3: Select a node by clicking on dom node element in AnimationTargetCompositor.

https://reviewboard.mozilla.org/r/235562/#review243774

::: devtools/client/inspector/animation/components/AnimationTarget.js:111
(Diff revision 2)
>          {
>            defaultRep: ElementNode,
>            mode: MODE.TINY,
>            inspectIconTitle: getInspectorStr("inspector.nodePreview.highlightNodeLabel"),
>            object: translateNodeFrontToGrip(nodeFront),
> +          onDOMNodeClick: () => setSelectedNode(nodeFront, { reason: "animation-panel" }),

Actually, I don't know if onDOMNodeClick should select the node. I expected this to be onInspectIconClick
Attachment #8966902 - Flags: review+
Comment on attachment 8966902 [details]
Bug 1453010 - Part 3: Select a node by clicking on dom node element in AnimationTargetCompositor.

https://reviewboard.mozilla.org/r/235562/#review243782

::: devtools/client/inspector/animation/components/AnimationTarget.js:111
(Diff revision 2)
>          {
>            defaultRep: ElementNode,
>            mode: MODE.TINY,
>            inspectIconTitle: getInspectorStr("inspector.nodePreview.highlightNodeLabel"),
>            object: translateNodeFrontToGrip(nodeFront),
> +          onDOMNodeClick: () => setSelectedNode(nodeFront, { reason: "animation-panel" }),

Ah disregard this as well, I see this is what it does in the current animation inspector. I would still consider adding scrollIntoView
Attachment #8966902 - Flags: review+
Comment on attachment 8966903 [details]
Bug 1453010 - Part 4: Add test for locking highlighting.

https://reviewboard.mozilla.org/r/235564/#review243904

::: devtools/client/inspector/animation/test/browser_animation_animation-target_highlight.js:19
(Diff revision 2)
> +//   the class will add to those animation target as well
> +
> +add_task(async function() {
> +  await addTab(URL_ROOT + "doc_simple_animation.html");
> +  await removeAnimatedElementsExcept([".animated", ".multi"]);
> +  const { animationInspector, panel } = await openAnimationInspector();

We can simplify this by either getting the inspector or toolbox from openAnimationInspector

and  do

toolbox.once("node-highlight")

::: devtools/client/inspector/animation/test/browser_animation_animation-target_highlight.js:21
(Diff revision 2)
> +add_task(async function() {
> +  await addTab(URL_ROOT + "doc_simple_animation.html");
> +  await removeAnimatedElementsExcept([".animated", ".multi"]);
> +  const { animationInspector, panel } = await openAnimationInspector();
> +
> +  info("Checking highlighting when mouse over on a target node");

s/Checking/Check/g

::: devtools/client/inspector/animation/test/browser_animation_animation-target_highlight.js:33
(Diff revision 2)
> +  let onUnhighlight = animationInspector.inspector.toolbox.once("node-unhighlight");
> +  mouseOutOnTargetNode(animationInspector, panel, 0);
> +  await onUnhighlight;
> +  ok(true, "Unhighlighted the targe node");
> +
> +  info("Checking locking highlighting when click on the inspect icon");

Check node is highlighted when the inspect icon is clicked.

::: devtools/client/inspector/animation/test/browser_animation_animation-target_highlight.js:39
(Diff revision 2)
> +  onHighlight = animationInspector.inspector.toolbox.once("node-highlight");
> +  await clickOnInspectIcon(animationInspector, panel, 0);
> +  nodeFront = await onHighlight;
> +  assertNodeFront(nodeFront, "DIV", "ball animated");
> +  ok(panel.querySelectorAll(".animation-target")[0].classList.contains("highlighting"),
> +    "The highlighting animation target element should have 'highlighting' class");

The highlighted animation target

::: devtools/client/inspector/animation/test/browser_animation_animation-target_highlight.js:41
(Diff revision 2)
> +  nodeFront = await onHighlight;
> +  assertNodeFront(nodeFront, "DIV", "ball animated");
> +  ok(panel.querySelectorAll(".animation-target")[0].classList.contains("highlighting"),
> +    "The highlighting animation target element should have 'highlighting' class");
> +
> +  info("Checking keeping to lock even if mouse is out from the animation target");

Check if the animation target is still highlighted on mouse out

::: devtools/client/inspector/animation/test/browser_animation_animation-target_highlight.js:45
(Diff revision 2)
> +
> +  info("Checking keeping to lock even if mouse is out from the animation target");
> +  mouseOutOnTargetNode(animationInspector, panel, 0);
> +  await wait(500);
> +  ok(panel.querySelectorAll(".animation-target")[0].classList.contains("highlighting"),
> +    "The highlighting element still should have 'highlighting' class");

The highlighted element

::: devtools/client/inspector/animation/test/browser_animation_animation-target_highlight.js:47
(Diff revision 2)
> +  mouseOutOnTargetNode(animationInspector, panel, 0);
> +  await wait(500);
> +  ok(panel.querySelectorAll(".animation-target")[0].classList.contains("highlighting"),
> +    "The highlighting element still should have 'highlighting' class");
> +
> +  info("Checking locking element after changing to the other");

info("Highlighting another animation target");

::: devtools/client/inspector/animation/test/browser_animation_animation-target_highlight.js:53
(Diff revision 2)
> +  onHighlight = animationInspector.inspector.toolbox.once("node-highlight");
> +  await clickOnInspectIcon(animationInspector, panel, 1);
> +  nodeFront = await onHighlight;
> +  assertNodeFront(nodeFront, "DIV", "ball multi");
> +
> +  info("Checking 'highlighting' class");

info("Check the highlighted state of the animation targets")

::: devtools/client/inspector/animation/test/head.js:202
(Diff revision 2)
>    EventUtils.synthesizeMouse(controllerEl, mousedonwX, 0, {}, controllerEl.ownerGlobal);
>    await waitForSummaryAndDetail(animationInspector);
>  };
>  
>  /**
> + * Click on an inspect icon in AnimationTargetComponent.

Reword this all to be:

Click on the inspect icon for the given AnimationTargetComponent

::: devtools/client/inspector/animation/test/head.js:205
(Diff revision 2)
>  
>  /**
> + * Click on an inspect icon in AnimationTargetComponent.
> + *
> + * @param {AnimationInspector} animationInspector.
> + * @param {AnimationsPanel} panel

I am really hesistant on keeping this. There's technically no object called AnimationsPanel anymore and really this is an DOMElement.

I would change all of this @param {AnimationsPanel} to @param {DOMElement} in this file or get rid of panel and do animationInspector.doc.querySelector everywhere.

::: devtools/client/inspector/animation/test/head.js:264
(Diff revision 2)
>    // Restore the scrubber style.
>    scrubberEl.style.pointerEvents = "unset";
>  };
>  
>  /**
> + * Click on a target node in AnimationTargetComponent.

Click on the target node for the given AnimationTargetComponent index.

::: devtools/client/inspector/animation/test/head.js:372
(Diff revision 2)
>    const rate = 1 / bounds.width * pixels;
>    return { duration, rate };
>  };
>  
>  /**
> + * Mouse over on a target node in AnimationTargetComponent.

Mouse over the target node for the given AnimationTargetComponent index.

::: devtools/client/inspector/animation/test/head.js:385
(Diff revision 2)
> +const mouseOverOnTargetNode = function(animationInspector, panel, index) {
> +  info(`Mouse over on a target node in animation target component[${ index }]`);
> +  const targetEl = panel.querySelectorAll(".animation-target .objectBox")[index];
> +  targetEl.scrollIntoView(false);
> +  EventUtils.synthesizeMouse(targetEl, 10, 5,
> +                             { type: "mouseover" }, targetEl.ownerGlobal);

Just a single tab is enough.

::: devtools/client/inspector/animation/test/head.js:389
(Diff revision 2)
> +  EventUtils.synthesizeMouse(targetEl, 10, 5,
> +                             { type: "mouseover" }, targetEl.ownerGlobal);
> +};
> +
> +/**
> + * Mouse out on a target node in AnimationTargetComponent.

Mouse out of the target node for the given AnimationTargetComponent index.

::: devtools/client/inspector/animation/test/head.js:402
(Diff revision 2)
> +const mouseOutOnTargetNode = function(animationInspector, panel, index) {
> +  info(`Mouse out on a target node in animation target component[${ index }]`);
> +  const targetEl = panel.querySelectorAll(".animation-target .objectBox")[index];
> +  targetEl.scrollIntoView(false);
> +  EventUtils.synthesizeMouse(targetEl, -1, -1,
> +                             { type: "mouseout" }, targetEl.ownerGlobal);

Just a single tab. We can even make this one line if you did const el instead of targetEl
Attachment #8966903 - Flags: review?(gl) → review+
Comment on attachment 8966900 [details]
Bug 1453010 - Part 1: Lock highlighted element by clicking on the inspect icon in AnimationTarget component.

https://reviewboard.mozilla.org/r/235558/#review243730

> This should be calling onShowBoxModelHighlighterForNode

Thank you for the review, Gabriel.

Unfortunately, although I have tried this way, onShowBoxModelHighlighterForNode did not work at here. Because, even if set the highlighted node by onShowBoxModelHighlighterForNode at here, when mouse was out of Rep component, onHideBoxModelHighlighter will be called. Naturally, the highlight was hidden.
Am I wrong something how to use?

I think we need an independent highlighter for locking as same as previous animation inspector[1].

[1] https://searchfox.org/mozilla-central/source/devtools/client/inspector/shared/dom-node-preview.js#325
(In reply to Daisuke Akatsuka (:daisuke) from comment #22)
> Comment on attachment 8966900 [details]
> Bug 1453010 - Part 1: Lock highlighted element by clicking on the inspect
> icon in AnimationTarget component.
> 
> https://reviewboard.mozilla.org/r/235558/#review243730
> 
> > This should be calling onShowBoxModelHighlighterForNode
> 
> Thank you for the review, Gabriel.
> 
> Unfortunately, although I have tried this way,
> onShowBoxModelHighlighterForNode did not work at here. Because, even if set
> the highlighted node by onShowBoxModelHighlighterForNode at here, when mouse
> was out of Rep component, onHideBoxModelHighlighter will be called.
> Naturally, the highlight was hidden.
> Am I wrong something how to use?
> 
> I think we need an independent highlighter for locking as same as previous
> animation inspector[1].
> 
> [1]
> https://searchfox.org/mozilla-central/source/devtools/client/inspector/
> shared/dom-node-preview.js#325

Maybe it's just weird that the target would toggle the box model highlighter in this case. We use highlighters-overlay.js to manage all our highlighter states because you can imagine if you navigate out of a page we need to clear this highlighter by hiding it and when it goes back to the page, it doesn't re-highlight it.
If we still want to proceed, I think you should implement the right methods in highlighters-overlay and send another review.
Alternatively, remove the onDOMNodeMouseOut and onDOMNodeMouseOver methods for the rep.
We can't remove onDOMNodeMouseOut, onDOMNodeMouseOver from the rep, because we want same feature for previous animation inspector.
Okay, I'll try to implement in highlighters-overlay.
Comment on attachment 8966902 [details]
Bug 1453010 - Part 3: Select a node by clicking on dom node element in AnimationTargetCompositor.

https://reviewboard.mozilla.org/r/235562/#review243744

> Consider also scrolling into view like https://searchfox.org/mozilla-central/source/devtools/client/inspector/grids/components/GridItem.js#86

Indeed, I'll do this. Thanks!
Let me cancel the r+ for patch 1, because I modified highlight-overlay.
Attachment #8966900 - Flags: review+
Attachment #8966900 - Flags: review?(gl)
Comment on attachment 8966900 [details]
Bug 1453010 - Part 1: Lock highlighted element by clicking on the inspect icon in AnimationTarget component.

https://reviewboard.mozilla.org/r/235558/#review246036

::: devtools/client/inspector/animation/animation.js:147
(Diff revision 3)
>      this.inspector.toolbox.on("picker-started", this.onElementPickerStarted);
>      this.inspector.toolbox.on("picker-stopped", this.onElementPickerStopped);
>      this.inspector.toolbox.on("select", this.onSidebarSelectionChanged);
>    }
>  
> -  destroy() {
> +  async destroy() {

I don't see any await inside of this destroy(). Do we need this async here?

::: devtools/client/inspector/animation/animation.js:449
(Diff revision 3)
>    setDetailVisibility(isVisible) {
>      this.inspector.store.dispatch(updateDetailVisibility(isVisible));
>    }
>  
>    /**
> +   * Set highlighted node. If set null, unhighlight.

Highlight the given node with the box model highlighter. If no node is provided, hide the box model highlighter.

::: devtools/client/inspector/animation/components/AnimationTarget.js:49
(Diff revision 3)
>        this.updateNodeFront(nextProps.animation);
>      }
>    }
>  
>    shouldComponentUpdate(nextProps, nextState) {
> -    return this.state.nodeFront !== nextState.nodeFront;
> +    return (this.state.nodeFront !== nextState.nodeFront) ||

I think this will be just as readable without the brackets. I would remove them.

::: devtools/client/inspector/shared/highlighters-overlay.js:411
(Diff revision 3)
> +    if (!isShown) {
> +      return;
> +    }
> +
> +    this.boxModelHighlighterShown = node;
> +    this.emit("box-model-highlighter-shown", node, options);

Same as abelow

::: devtools/client/inspector/shared/highlighters-overlay.js:424
(Diff revision 3)
> +      return;
> +    }
> +
> +    await this.highlighters.BoxModelHighlighter.hide();
> +    this.boxModelHighlighterShown = null;
> +    this.emit("box-model-highlighter-hidden");

I don't see us using this emitted event. So, maybe remove it for now.
Attachment #8966900 - Flags: review?(gl) → review+
Comment on attachment 8966900 [details]
Bug 1453010 - Part 1: Lock highlighted element by clicking on the inspect icon in AnimationTarget component.

https://reviewboard.mozilla.org/r/235558/#review246036

> I don't see any await inside of this destroy(). Do we need this async here?

Thanks, Gabriel.

Ah, that's the remains of previous changes. Thanks.

> I don't see us using this emitted event. So, maybe remove it for now.

Okay, thanks!
I changed the test a bit so as to wait for highlighting when the animation target clicking.
And try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=586881a1d384235eecbf39862f093cf56b7e53cb
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bc816979fee8
Part 1: Lock highlighted element by clicking on the inspect icon in AnimationTarget component. r=gl
https://hg.mozilla.org/integration/autoland/rev/407a33054ca6
Part 2: Change the title of inspect icon. r=gl
https://hg.mozilla.org/integration/autoland/rev/a1666927ab16
Part 3: Select a node by clicking on dom node element in AnimationTargetCompositor. r=gl
https://hg.mozilla.org/integration/autoland/rev/cd392b05865e
Part 4: Add test for locking highlighting. r=gl
Verified on Win10 and MacOS 10.12 on Nightly 61.0a1(BuildID 20180502100112), the issue is fixed.
Status: RESOLVED → VERIFIED
Thanks for verifying this, also marking the status of 61 as fixed based on your testing.
status-firefox61: fixed → verified
Removing tracking flag since the feature is pref'ed off in FX61 and has been moved to Fx62
tracking-firefox61: + → ---
Product: Firefox → DevTools
status-firefox59: unaffected → ---
Flags: in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: