Closed Bug 1450927 Opened 6 years ago Closed 6 years ago

The purple highlight overlay is misplaced in Responsive Design mode

Categories

(DevTools :: Accessibility Tools, defect)

61 Branch
defect
Not set
normal

Tracking

(firefox61 verified)

VERIFIED FIXED
Firefox 61
Tracking Status
firefox61 --- verified

People

(Reporter: tbabos, Assigned: yzen)

References

Details

Attachments

(3 files, 3 obsolete files)

[Affected versions]:
Nightly 61.0a1

[Affected Platforms]:
Ubuntu 16.04 x64, Windows 10 x64

[Prerequisites]:
- Have the latest try build 61.0a1 from (2018-03-27) installed
- Accessibility is enabled and accessibility tab is opened
- Responsive Design mode is enabled

[Steps to reproduce]:
1. Select any of the listed devices resolution
2. Click the "Pick accessible objects from the page" button
3. Hover over any element from the page's content

[Expected result]:
The element inspected with Accessibility should be highlighted with a purple transparent overlay.

[Actual result]:
The purple highlight overlay is misplaced or it is not displayed at all.
(In reply to Timea Babos from comment #0)
> Created attachment 8964524 [details]
> Screenshot of the issue - 2018 -04-03
> 
> [Affected versions]:
> Nightly 61.0a1
> 
> [Affected Platforms]:
> Ubuntu 16.04 x64, Windows 10 x64
> 
> [Prerequisites]:
> - Have the latest try build 61.0a1 from (2018-03-27) installed
> - Accessibility is enabled and accessibility tab is opened
> - Responsive Design mode is enabled
> 
> [Steps to reproduce]:
> 1. Select any of the listed devices resolution
> 2. Click the "Pick accessible objects from the page" button
> 3. Hover over any element from the page's content
> 
> [Expected result]:
> The element inspected with Accessibility should be highlighted with a purple
> transparent overlay.
> 
> [Actual result]:
> The purple highlight overlay is misplaced or it is not displayed at all.

Hi Timea,

Could you confirm that this only happens when the DPR is changed, not the resolution? If that's not the case, could you provide what values you have as defaults and what values do you change to?
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
Flags: needinfo?(timea.babos)
Hey Yura,

The highlight is placed correctly with the following default values:
- No throttling
- DPR: 1
- No device selected
- None of the options in “Reload when...” are checked

The issue occurs if either the DPR is changed to other values (DPR:2; DPR:3) or any of the listed devices resolution is selected, thus the DPR option is automatically changed according to the selected device. 
If any custom resolution is created by dragging the window and resizing manually, the highlight will be properly displayed. As you said, it occurs only if the DPR option is changed to any possible value, except the default DPR:1. 
Hope this was helpful, please ask me for any further information you need.
Flags: needinfo?(timea.babos)
(In reply to Timea Babos from comment #2)
> Hey Yura,
> 
> The highlight is placed correctly with the following default values:
> - No throttling
> - DPR: 1
> - No device selected
> - None of the options in “Reload when...” are checked
> 
> The issue occurs if either the DPR is changed to other values (DPR:2; DPR:3)
> or any of the listed devices resolution is selected, thus the DPR option is
> automatically changed according to the selected device. 
> If any custom resolution is created by dragging the window and resizing
> manually, the highlight will be properly displayed. As you said, it occurs
> only if the DPR option is changed to any possible value, except the default
> DPR:1. 
> Hope this was helpful, please ask me for any further information you need.

Thanks, very helpful.
Attached patch 1450927 prelimeary patch (obsolete) — Splinter Review
Attachment #8969390 - Flags: review?(surkov.alexander)
Attached patch 1450927 accessible patch (obsolete) — Splinter Review
Alex for accessible/ review
Jimm to take a look at the ipc/ipdl/sync-messages.ini changes
Attachment #8969390 - Attachment is obsolete: true
Attachment #8969390 - Flags: review?(surkov.alexander)
Attachment #8969769 - Flags: review?(surkov.alexander)
Attachment #8969769 - Flags: review?(jmathies)
Attachment #8969770 - Flags: review?(jdescottes)
Comment on attachment 8969769 [details] [diff] [review]
1450927 accessible patch

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

::: accessible/generic/Accessible.cpp
@@ +693,5 @@
> +Accessible::BoundsInCSSPixels() const
> +{
> +  nsRect bounds = BoundsInAppUnits();
> +  nsPresContext* presContext = mDoc->PresContext();
> +  return bounds.ToNearestPixels(presContext->AppUnitsPerCSSPixel());

it makes sense to make these two functions inline and also it can be written a shorter way:
BoundsInAppUnits().ToNearestPixels(mDoc->PresContext()->AppUnitsPerCSSPixel());

::: accessible/generic/Accessible.h
@@ +511,5 @@
>  
> +  /**
> +   * Return boundaries in screen coordinates in app units.
> +   */
> +  virtual nsRect BoundsInAppUnits() const;

ah, so no sense to make those inline until you de-vertualize them

::: accessible/html/HTMLListAccessible.cpp
@@ +88,5 @@
>    return rect;
>  }
>  
> +nsIntRect
> +HTMLLIAccessible::BoundsInCSSPixels() const

it'd be nicer to convert Bounds() into BoundsInAppPixels(), since it allows you to avoid code duplication, and also makes you closer to de-vertualizing BoundsInCSSPixels

::: accessible/interfaces/nsIAccessible.idl
@@ +28,5 @@
>   * nodes in the DOM tree -- such as documents, focusable elements and text.
>   * Mozilla creates the implementations of nsIAccessible on demand.
>   * See http://www.mozilla.org/projects/ui/accessibility for more information.
>   */
> +[scriptable, builtinclass, uuid(ea662d70-4311-11e8-b566-0800200c9a66)]

I vaguely recall that we no longer required to change uuids, I can't see how it can harm, but maybe it makes sense to double check that with someobdy who knows

@@ +225,5 @@
>    nsIArray getRelations();
>  
>    /**
>     * Return accessible's x and y coordinates relative to the screen and
>     * accessible's width and height.

worth to mention that these are in dev pixels

@@ +233,5 @@
> +  /**
> +   * Return accessible's x and y coordinates relative to the screen and
> +   * accessible's width and height in CSS pixels.
> +   */
> +  void getBoundsInCSSPixels(out long aX, out long aY, out long aWidth, out long aHeight);

curious if DOMRect will be a better match

::: accessible/tests/mochitest/bounds/test_list.html
@@ +46,5 @@
>           "Outside list item y should match to list item element y");
>        ok(widthLI > widthLIElm,
>           "Outside list item width=" + widthLI + " should be greater than list item element width=" + widthLIElm);
> +      ok(heightLI > heightLIElm,
> +         "Outside list item height=" + heightLI + " should be greater than list item element height=" + heightLIElm);

I presume this is because of new ToNearestPixels, so could you convert these to test the difference is not bigger than 1?

::: accessible/tests/mochitest/layout.js
@@ +186,5 @@
>  }
>  
>  /**
>   * Return the accessible coordinates and size relative to the screen in device
>   * pixels.

might be nice to adjust the comment to mention this method tests css vs dev boundaries as well

@@ +196,5 @@
>    accessible.getBounds(x, y, width, height);
> +  accessible.getBoundsInCSSPixels(xInCSS, yInCSS, widthInCSS, heightInCSS);
> +
> +  const { devicePixelRatio } = window;
> +  ok(Math.round(x.value / devicePixelRatio) >= xInCSS.value, "X in CSS pixels is calculated correctly");

>= for error of 1 looks scary, so this ceil function might work well:
is(Math.ceil(x.value / devicePixelRatio), xInCSS.value, "bla") or Math.trunct might work well.

If it doesn't, then
ok(Math.abs(x.value / devicePixelRatio - xInCSS.value) <= 1, "bla");

::: accessible/xul/XULTreeAccessible.cpp
@@ +722,5 @@
>    return FocusMgr()->FocusedAccessible() == this ? this : nullptr;
>  }
>  
>  nsIntRect
> +XULTreeItemAccessibleBase::BoundsRect() const

so this one actually should be BoundsInCSSPixels, and also you should provide BoundsInAppPixels for correctness. Same applied to XULGridTree
Comment on attachment 8969770 [details] [diff] [review]
1450927 devtools patch

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

I won't be able to build and review this quickly, forwarding to Ryan.
Attachment #8969770 - Flags: review?(jdescottes) → review?(jryans)
Comment on attachment 8969769 [details] [diff] [review]
1450927 accessible patch

canceling review until comments are addressed
Attachment #8969769 - Flags: review?(surkov.alexander)
Attached patch 1450927 accessible patch v2 (obsolete) — Splinter Review
Marking Alex for a11y parts.

Marking Jed for changes in ipc/ipdl/sync-messages.ini
Some context: this method is similar to PDocAccessible::Extents but for CSS pixels.
Attachment #8969769 - Attachment is obsolete: true
Attachment #8969769 - Flags: review?(jmathies)
Attachment #8970674 - Flags: review?(surkov.alexander)
Attachment #8970674 - Flags: review?(jld)
Comment on attachment 8969770 [details] [diff] [review]
1450927 devtools patch

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

Thanks, this looks reasonable to me! :)
Attachment #8969770 - Flags: review?(jryans) → review+
Comment on attachment 8970674 [details] [diff] [review]
1450927 accessible patch v2

The sync message change seems reasonable: it's equivalent to adding a parameter to the existing sync message PDocAccessible::Extents.
Attachment #8970674 - Flags: review?(jld) → review+
Rebased a11y changes with Eitan's patch.
Attachment #8970674 - Attachment is obsolete: true
Attachment #8970674 - Flags: review?(surkov.alexander)
Attachment #8970791 - Flags: review?(surkov.alexander)
Comment on attachment 8970791 [details] [diff] [review]
1450927 accessible patch v3

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

::: accessible/generic/Accessible.h
@@ +517,4 @@
>    /**
>     * Return boundaries in screen coordinates.
>     */
>    virtual nsIntRect Bounds() const;

if you remove XULTree/XULTreeGrid overridings, then you can devirtualize the method and make it inline

::: accessible/ipc/other/ProxyAccessible.cpp
@@ +1017,5 @@
> +ProxyAccessible::BoundsInCSSPixels()
> +{
> +  nsIntRect rect;
> +  Unused << mDoc->SendExtentsInCSSPixels(mID,
> +                                       &(rect.x), &(rect.y),

nit: do you really need braces around rect.x and so?

btw, indentation seems broken

::: accessible/ipc/win/ProxyAccessible.cpp
@@ +245,5 @@
> +
> +  long x;
> +  long y;
> +  long width;
> +  long height;

I prefer to initialize local variables

@@ +251,5 @@
> +  if (FAILED(hr)) {
> +    return nsIntRect();
> +  }
> +
> +  return nsIntRect(x, y, width, height);

you could do same as you do in ProxyAccessible and get rid of x, y ... local variables by using nsIntRect directly.

::: accessible/tests/mochitest/layout.js
@@ +210,5 @@
> +  accessible.getBoundsInCSSPixels(xInCSS, yInCSS, widthInCSS, heightInCSS);
> +
> +  const { devicePixelRatio } = window;
> +  ok(Math.ceil(x.value / devicePixelRatio) >= xInCSS.value,
> +    "X in CSS pixels is calculated correctly");

So ceil/trunc didn't work, and the difference can be > 0?

::: accessible/xul/XULTreeAccessible.cpp
@@ +754,5 @@
> +  return nsIntRect(x, y, width, height);
> +}
> +
> +nsIntRect
> +XULTreeItemAccessibleBase::Bounds() const

I think it should be ok to not override it, Accessible::Bounds() will work as long as you implement BoundsInAppUnits()
Attachment #8970791 - Flags: review?(surkov.alexander) → review+
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7dd8b0d2473
add getBoundsInCSSPixels XPCOM method. r=surkov r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca285aed3926
use new getBoundsInCSSPixels method for accessible bounds in accessible highlighter. r=jryans
Backout by toros@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89bc67f5eaaf
Backed out 2 changesets for failing on ProxyAccessible.cpp(247) on a CLOSED TREE
https://hg.mozilla.org/integration/mozilla-inbound/rev/89bc67f5eaaf6348c102e2b0aa6152b87b9c99cc

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ca285aed3926966562f9444b912eb24d757d2657

https://treeherder.mozilla.org/logviewer.html#?job_id=175551691&repo=mozilla-inbound&lineNumber=26540

16:13:38     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/toolkit/components/find'
16:13:38     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/toolkit/components/find'
16:13:38     INFO -  toolkit/components/find
16:13:38     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/toolkit/components/find'
16:13:38     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/toolkit/components/find'
16:13:38     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/toolkit/components/find'
16:13:38     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/accessible/ipc/win'
16:13:38     INFO -  z:/build/build/src/sccache2/sccache.exe z:/build/build/src/vs2017_15.6.6/VC/bin/Hostx64/x64/cl.exe -FoProxyAccessible.obj -c -Iz:/build/build/src/obj-firefox/dist/stl_wrappers -DNDEBUG=1 -DTRIMMED=1 -DWIN32_LEAN_AND_MEAN -D_WIN32 -DWIN32 -D_CRT_RAND_S -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -DOS_WIN=1 -D_UNICODE -DCHROMIUM_BUILD -DU_STATIC_IMPLEMENTATION -DUNICODE -D_WINDOWS -D_SECURE_ATL -DCOMPILER_MSVC -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -Iz:/build/build/src/accessible/ipc/win -Iz:/build/build/src/obj-firefox/accessible/ipc/win -Iz:/build/build/src/accessible/base -Iz:/build/build/src/accessible/generic -Iz:/build/build/src/accessible/windows/ia2 -Iz:/build/build/src/accessible/windows/msaa -Iz:/build/build/src/accessible/xpcom -Iz:/build/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -Iz:/build/build/src/ipc/chromium/src -Iz:/build/build/src/ipc/glue -Iz:/build/build/src/obj-firefox/dist/include -Iz:/build/build/src/obj-firefox/dist/include/nspr -Iz:/build/build/src/obj-firefox/dist/include/nss -MD -FI z:/build/build/src/obj-firefox/mozilla-config.h -DMOZILLA_CLIENT -utf-8 -TP -nologo -w15038 -wd5026 -wd5027 -Zc:sizedDealloc- -wd4091 -wd4577 -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -Gw -wd4251 -wd4244 -wd4267 -wd4800 -wd4595 -we4553 -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING -GR- -Z7 -O1 -Oi -Oy- -WX  -deps.deps/ProxyAccessible.obj.pp    z:/build/build/src/accessible/ipc/win/ProxyAccessible.cpp
16:13:38     INFO -  ProxyAccessible.cpp
16:13:38     INFO -  z:/build/build/src/accessible/ipc/win/ProxyAccessible.cpp(247): error C2664: 'HRESULT IGeckoCustom::get_boundsInCSSPixels(long *,long *,long *,long *)': cannot convert argument 1 from 'int32_t *' to 'long *'
16:13:38     INFO -  z:/build/build/src/accessible/ipc/win/ProxyAccessible.cpp(247): note: Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast
16:13:38     INFO -  z:/build/build/src/config/rules.mk:1024: recipe for target 'ProxyAccessible.obj' failed
16:13:38     INFO -  mozmake.EXE[4]: *** [ProxyAccessible.obj] Error 2
16:13:38     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/accessible/ipc/win'
16:13:38     INFO -  z:/build/build/src/config/recurse.mk:73: recipe for target 'accessible/ipc/win/target' failed
16:13:38     INFO -  mozmake.EXE[3]: *** [accessible/ipc/win/target] Error 2
16:13:38     INFO -  mozmake.EXE[3]: *** Waiting for unfinished jobs....
16:13:38     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/toolkit/components/find'
16:13:38     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/toolkit/components/find'
16:13:38     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/toolkit/components/find'
16:13:38     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/toolkit/components/find'
16:13:39     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/accessible/xul'
Flags: needinfo?(yzenevich)
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b58c8b754ce2
add getBoundsInCSSPixels XPCOM method. r=surkov r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c0476f1f505
use new getBoundsInCSSPixels method for accessible bounds in accessible highlighter. r=jryans
Should be fixed now ...
Flags: needinfo?(yzenevich)
https://hg.mozilla.org/mozilla-central/rev/b58c8b754ce2
https://hg.mozilla.org/mozilla-central/rev/1c0476f1f505
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Depends on: 1457148
On the latest Firefox Nightly (Build ID: 20180426100055) the issue cannot be reproduced on Mac OS X 10.12, Windows 10 x64 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.