Trevor Saunders (:tbsaunde) Assignee
	Description • 9 years ago

      No description provided.

	Trevor Saunders (:tbsaunde) Assignee
	Comment 1 • 9 years ago

if this is useful it would make more sense to log it outside of the windows
layer.  Since its not clear it is useful, and it makes it harder to separate
event dispatch logic from HandleAccEvent its easiest to just remove it for now.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 4 • 9 years ago

Comment on attachment 8654240 [details] [diff] [review]
remove event logging from the windows AccessibleWrap::HandleAccEvent

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

r=me assuming surkov is ok with it?

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 5 • 9 years ago

Comment on attachment 8654241 [details] [diff] [review]
factor win event dispatch logic into its own function

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

::: accessible/windows/msaa/AccessibleWrap.cpp
@@ +1227,5 @@
> +  if (aEventType == nsIAccessibleEvent::EVENT_SELECTION &&
> +      Compatibility::IsJAWS()) {
> +    roles::Role role = aTarget->IsProxy() ? aTarget->Proxy()->Role() :
> +      aTarget->Role();
> +       if (role == roles::COMBOBOX_OPTION) {

Please remove 3 indent spaces in line above.

@@ +1334,5 @@
>      return nullptr;
>    }
>  
>    // Accessibles in child processes are said to have the HWND of the window
> +  // there tab is within.  Popups are always in the parent process, and so

nit: "there" should "their" (I know it was misspelled previously as well). I guess this these HWNDs are going away...

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 6 • 9 years ago

Comment on attachment 8654242 [details] [diff] [review]
fire windows events on proxies

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

LGTM, nits:

::: accessible/windows/msaa/Platform.cpp
@@ +80,3 @@
>  {
> +  AccessibleWrap* wrapper = WrapperFor(aTarget);
> +  AccessibleWrap::FireWinEvent(wrapper, aEventType);

Not sure what the variable gains us here.

@@ +87,4 @@
>  {
> +  AccessibleWrap* wrapper = WrapperFor(aTarget);
> +  AccessibleWrap::FireWinEvent(wrapper,
> +                               nsIAccessibleEvent::EVENT_STATE_CHANGE);

Not sure what the variable gains us here.

@@ +93,5 @@
>  void
>  a11y::ProxyCaretMoveEvent(ProxyAccessible* aTarget, int32_t aOffset)
>  {
> +  AccessibleWrap* wrapper = WrapperFor(aTarget);
> +  AccessibleWrap::FireWinEvent(wrapper, nsIAccessibleEvent::EVENT_TEXT_CARET_MOVED);

Not sure what the variable gains us here.

	alexander :surkov (:asurkov)
	Comment 7 • 9 years ago

(In reply to David Bolter [:davidb] from comment #4)
> Comment on attachment 8654240 [details] [diff] [review]
> remove event logging from the windows AccessibleWrap::HandleAccEvent
> 
> Review of attachment 8654240 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me assuming surkov is ok with it?

we chatted on irc on it. I used that in the past couple times, but I didn't for a while. Trevor convinced me the code can be reintroduced later at not big cost, if needed.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 8 • 9 years ago

All good then :)

	Pulsebot
	Comment 9 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/94f7fbe3876a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f45bbef7719d
https://hg.mozilla.org/integration/mozilla-inbound/rev/078fc05c0a31

	Carsten Book [:Tomcat]
	Comment 10 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/94f7fbe3876a
https://hg.mozilla.org/mozilla-central/rev/f45bbef7719d
https://hg.mozilla.org/mozilla-central/rev/078fc05c0a31