352093 - (widget-removal) Remove child widgets from content area

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We can simplify a lot of code and probably get a significant performance win by removing usage of native widgets everywhere except for the toplevel window and plugins.

This will make view removal much easier so it would be best if it happened before view removal.

Work that this depends on includes bug 339548 (get plugin widgets up to the top of their widget hierarchy) and bug 130078 (integrate view manager hierarchies so there's one view manager hierarchy per toplevel window).

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Did you forget about bug 316489? :)

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

yes I did!

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

*** Bug 316489 has been marked as a duplicate of this bug. ***

Comment 4

[Marian POPESCU]

Could bug 369599 be solved by this ?

Comment 5

[Scott]

(In reply to comment #0)
> We can simplify a lot of code and probably get a significant performance win by
> removing usage of native widgets everywhere except for the toplevel window and
> plugins.

By "native" are you referring to OS-native?

I hope not.

Comment 6

[Boris Zbarsky [:bzbarsky]]

This bug isn't about user interface widgets (buttons, checkboxes, etc).  It's about native windows being used as drawing areas.

Comment 7

[Scott]

(In reply to comment #6)
> This bug isn't about user interface widgets (buttons, checkboxes, etc).  It's
> about native windows being used as drawing areas.
> 

Uhhh. OK.

Nevermind. :)

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I have this implemented. I'm going to file supporting bugs for various pieces of preliminary work that can land on their own. A great deal of the work will end up in bug 339548. This bug will contain two large patches: a patch to remove widgets for IFRAMEs, and a patch to remove widgets for scrollframes.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Add view parameter to nsViewManager::DispatchEvent

PresShell::DispatchSynthMouseMove needs to be able to dispatch an nsGUIEvent to a particular document. That breaks if that document has no widget, since the widget alone is no longer enough information to target the right document. So this patch adds a view parameter to nsViewManager::DispatchEvent. This avoids unnecessary GetViewFor calls in DispatchEvent too.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 386159 [details] [diff] [review]
Part 1: Add view parameter to nsViewManager::DispatchEvent

>+++ b/view/src/nsView.cpp
>-    vm->DispatchEvent(aEvent, &result);
>+    view->GetViewManager()->DispatchEvent(aEvent, view, &result);

Why the change from "vm" to "view->GetViewManager()" here?

Apart from that, looks good.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Probably a merge issue. I'll fix it before checking in. Thanks.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Create nsIViewManager::GetRootWidget

Quite a few places in our code call nsIViewManager::GetWidget to get the widget at the root of the document, and will be surprised when that returns null. So let's create GetRootWidget, which gets the root widget for the document if there is one, otherwise returns the root widget for the nearest enclosing document that has one.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: fix accessibility to not require a widget per document

nsAccessible::GetRelationByType needs to add NODE_CHILD_OF relations for document roots even when the document root has no widget.

nsDocAccessible's constructor will only set mWnd if the document has a widget. I think that's OK. I'm changing this here because nsIViewManager::GetWidget is going to go away.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4: IME code should use GetRootWidget

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 5: the focus manager should use GetRootWidget

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 6: fix GetScreenRect to handle appunits-per-dev-pixel changes across document boundaries where there's no widget

In some mochitests we set the zoom factor of a content iframe, so the iframe has a different appunits-per-dev-pixel value from its parent. When every document has a widget that's OK because we use the nearest widget to convert to screen coords, and that widget is always in the same document. But if there's no widget at the root, nsFrame::GetScreenRect goes wrong because it implicitly assumes that that value is the same all the way up the frame tree.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 7: fix nsXULPopupManager's saved mouse coordinates

nsXULPopupManager saves mouse coordinates from an event and uses them later for another event. It assumes that "client coordinates" are interpreted relative to the same origin (the viewport top-left) as the top-left of the nearest widget for the document's root frame, which is no longer going to be true. So store these coordinates relative to the GetRootWidget().

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 8: fix nsCaret::GetViewForRendering

GetViewForRendering fails to return the toplevel document's root view, instead it returns the current document's root view. withinViewOffset is already correctly set to be relative to the toplevel document's root view. This is an unrelated drive-by bug fix. (Since only nsContentEventHandler uses eTopLevelWindowCoordinates, this could only have affected some IMEs in IFRAMEs.)

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 9: fix context menu event coordinates

AdjustContextMenuKeyEvent sets the widget and coordinates of a mouse event to control where the context menu pops up. Currently it uses the document's widget. Instead, let's use the widget for the document at the top of the view hierarchy.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 10: misc fixes

-- nsContentSink just needs any old widget. Use GetRootWidget.
-- nsEventStateManager has some debug-only code that's unused even in there.
-- nsGlobalChromeWindow::SetCursor needs to find a widget to set the cursor on. Use GetRootWidget.
-- I have no idea what nsWidgetUtils::UpdateFromEvent is for, but aWidget and aViewManager are always null, so remove them. mWidget is only used as the widget to construct a scroll event, and we dispatch to the correct view automatically (thanks to patch 1) so I think using GetRootWidget here is fine.
-- nsObjectFrame has some gnarly code for Windows to figure out which widget to return when a windowless plugin asks for one. I'm just hacking that code here to return the nearest document widget instead of the current document widget, but the good news is that the work in this bug will mean we can actually remove this code completely! This code chooses between returning the "nearest widget" and the "nearest document widget", and after this bug is fixed they will always be the same!

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 11: remove nsViewManager calls to nsViewManager::GetWidget

-- nsIView::GetNearestWidget can just return null if it was unable to find a view with a widget in its ancestor chain. Calling GetWidget won't find anything; if the root view had a widget, we'd have found it already.
-- nsViewManager::DispatchEvent can just use GetRootWidget instead of GetWidget since it just wants to find a widget to check for transparency.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 12: Teach nsDocumentViewer and nsDocShell to deal with not having a widget

This is a bit more interesting. Fortunately docshell is pretty easy; we just need to avoid checking that its parent widget is non-null. Also, get pixel bounds from the content viewer instead of checking the widget of its view manager.

nsDocumentViewer's mWindow is now allowed to be null. We store explicit bounds in the docviewer since the widget might not be around. CreateDeviceContext takes a view parameter from which it can find a widget, instead of getting the widget directly. If someone calls SetBounds and there's no widget, we call nsIViewManager::SetWindowDimensions directly instead of having the widget do an NS_SIZE event. FindContainerView gets more complicated ... we find the container view by locating the frame for the document-embedding content and getting the view from it.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 13: remove nsIViewManager::GetWidget

At this point we've removed all uses of nsIViewManager::GetWidget, so get rid of it since anyone trying to use it is almost certainly broken.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 14: make content IFRAMEs windowless

Everything else has been building up to this. This is the patch that actually makes content IFRAMEs windowless.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 386178 [details] [diff] [review]
Part 4: IME code should use GetRootWidget

I don't test these patches actually, but looks ok for me.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 15: make scrollframes windowless

This is the other hard bit. The main thing here is that nsIWidget::Scroll changes dramatically; it now takes a rectangle to be moved, and a list of child widgets to be moved, resized and clipped to given regions. So there are Windows, GTK2 and Mac implementations of the new API.

The GTK2 implementation is roughly described here:
http://weblogs.mozillazine.org/roc/archives/2009/06/stupid_x_tricks.html

The Windows implementation is quite different. On Windows we use SW_SCROLLCHILDREN if all children are moving. We shrink the clip regions of all changing children before we move them, then enlarge them afterward if necessary, so that bits of windows don't stick out further than they should.

The Mac implementation is easy and mostly just removes code that's no longer needed.

nsScrollPortView changes a fair bit. We make ViewPositionDidChange return a list of configuration changes (nsGfxScrollFrame asks the RootPresContext for the desired plugin widget changes). Those changes are passed on down to nsIWidget::Scroll(). We had Windows-only code that finds the largest rectangle in the scrollable area which should actually be scrolled (very useful when there is a fixed-pos header or footer bar which we don't really want to scroll); this is now enabled on all platforms since we have cross-platform ability to scroll an arbitrary rectangle. One subtle thing is that nsScrollPortView is now responsible for invalidating the area that has been newly scrolled into view, so there's a bit of code added ("// Compute the area that's being exposed ...")

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 386202 [details] [diff] [review]
Part 15: make scrollframes windowless

Guess I'll ask dbaron for review on the view/layout parts, Josh for Mac, Karl to GTK2, and Jim Mathies for Windows.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 386202 [details] [diff] [review]
Part 15: make scrollframes windowless

Josh for the Mac changes...

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 386202 [details] [diff] [review]
Part 15: make scrollframes windowless

Karl for GTK2

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 386202 [details] [diff] [review]
Part 15: make scrollframes windowless

Jim for Windows

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I've published all the patches leading from mozilla-central up to and including this bug here:
http://hg.mozilla.org/users/rocallahan_mozilla.com/compositor-patches/

Comment 32

[alexander :surkov (:asurkov)]

It must be worth to create try server build and test it with Windows screen
readers since they rely on HWNDs and if we are going to get rid most of HWNDs
then it might affect on screen readers. Marco, could you please perform testing
of these patches?

James, any thoughts how it can affect on NVDA?

Comment 33

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> Created an attachment (id=386177) [details]
> Part 3: fix accessibility to not require a widget per document
> 
> nsAccessible::GetRelationByType needs to add NODE_CHILD_OF relations for
> document roots even when the document root has no widget.

It might be not needed any more because now we provide node_child_of relation because accessible tree is changed by Windows, i.e. when we had widget with HWND then Windows creates accessible that is not linked with Gecko a11y tree correctly. Therefore we use this relation to "repair" the accessible tree.

James, could you comment please?

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I published builds here:
https://build.mozilla.org/tryserver-builds/[e-mail]-try-f9bf8fa4178/
These aren't quite the latest patches but the only things I've fixed have been repainting bugs. MarcoZ apparently tried them and they worked OK.

Comment 35

[Marco Zehe (:MarcoZ)]

I tested a build recently with both JAWS and Window-Eyes, as well as NVDA on Windows, and they all seem to be using the non-hwnd-dependent NAVRELATION_EMBEDS relation as documented here: http://www.mozilla.org/access/windows/at-apis#relation_embeds. I didn't notice any breackage with current versions of JAWS 10, WE 7.1 and NVDA.

Comment 36

[alexander :surkov (:asurkov)]

Marco, are tests from test_relations.xul passed? There we have a test for node_child_of relation for windows.

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

roc, IMEs almost work fine on your tryserver build (I confirmed on Win32).

However, IMEs (both IMM32 and TSF) don't work fine only on the rich format editor of GMail. The composition string is committed immediately. Looks like that the editor of designMode=true doesn't work fine. But contentediable works fine for me.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

roc:

I checked the log of nsIMM32Handler. By the log, query content events are failed on the designMode editors. Would you check the related code of the event handling?

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #36)
> Marco, are tests from test_relations.xul passed?

Yes. The builds pass all tests.

(In reply to comment #38)
> I checked the log of nsIMM32Handler. By the log, query content events are
> failed on the designMode editors. Would you check the related code of the event
> handling?

My guess is that we're dispatching the query content events at the top level document (since it's the only document with a widget), and nsContentEventHandler is only looking at that document. It needs to look at the document that has focus. Perhaps the right thing to do is to forward query content events to the focused element the same way we do for IME and key events, here?
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6099

It would be great if you could make your own build. Just apply the patches from http://hg.mozilla.org/users/rocallahan_mozilla.com/compositor-patches on top of changeset 7a8502b70fdf from mozilla-central.

Comment 40

[alexander :surkov (:asurkov)]

(In reply to comment #39)
> (In reply to comment #36)
> > Marco, are tests from test_relations.xul passed?
> 
> Yes. The builds pass all tests.

So we still have HWND for window of iframe, but you got rid windows for scrollable areas, for example, do I understand right?

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No. IFRAMEs do not have HWNDs. See "Part 14".

Comment 42

[alexander :surkov (:asurkov)]

(In reply to comment #41)
> No. IFRAMEs do not have HWNDs. See "Part 14".

Does nsIWidget have HWND? If so do you have any idea why this works (http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/test_relations.xul#66)

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #42)
> Does nsIWidget have HWND? If so do you have any idea why this works
> (http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/test_relations.xul#66)

Yes, on Windows every nsIWidget has an HWND. But with my work in this bug, content IFRAMEs have no associated nsIWidget.

That test works because of my "Part 3" patch:
-          if (scrollFrame || view->GetWidget()) {
+          if (scrollFrame || view->GetWidget() || !frame->GetParent()) {
We provide a NODE_CHILD_OF relation for every IFRAME's nsDocAccessible, because nsDocAccessible::GetFrame() returns the document's root frame, and the root frame's GetParent() is null.

Comment 44

[alexander :surkov (:asurkov)]

(In reply to comment #43)
> (In reply to comment #42)
> > Does nsIWidget have HWND? If so do you have any idea why this works
> > (http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/test_relations.xul#66)
> 
> Yes, on Windows every nsIWidget has an HWND. But with my work in this bug,
> content IFRAMEs have no associated nsIWidget.
> 
> That test works because of my "Part 3" patch:

Ok, I see. Node child of relation don't make sense for iframes any more I
think.

If I have few windows open (for example, Firefox and DOM Inspector) then do
they have own HWND's? If so then I think we should expose node_child_of
relation for them only.

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #44)
> Ok, I see. Node child of relation don't make sense for iframes any more I
> think.

OK. If it's OK with you, then I would prefer to not change that behaviour in this bug, just preserve it using my existing patch. If you want to remove those relations then you can do that in a separate bug.

Comment 46

[alexander :surkov (:asurkov)]

(In reply to comment #45)
> (In reply to comment #44)
> > Ok, I see. Node child of relation don't make sense for iframes any more I
> > think.
> 
> OK. If it's OK with you, then I would prefer to not change that behaviour in
> this bug, just preserve it using my existing patch.

Could you upload new patch so that Marco could try?

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Nothing has changed related to accessiblity since the builds I mentioned in comment #34.

Comment 48

[James Teh [:Jamie]]

This completely breaks NVDA for documents containing frames; the document renders as blank. I haven't quite worked out why this is the case yet...

Comment 49

[James Teh [:Jamie]]

(In reply to comment #44)
> Node child of relation don't make sense for iframes any more I
> think.
I can confirm that this isn't necessary any more in theory, as retrieving the parent of the sub-document now correctly returns its frame. However, NVDA currently depends on this relation and will break if it is removed. I'd be willing to consider changing this, but I think this is a separate issue.

Comment 50

[alexander :surkov (:asurkov)]

(In reply to comment #49)
> (In reply to comment #44)
> > Node child of relation don't make sense for iframes any more I
> > think.
> I can confirm that this isn't necessary any more in theory, as retrieving the
> parent of the sub-document now correctly returns its frame. However, NVDA
> currently depends on this relation and will break if it is removed. I'd be
> willing to consider changing this, but I think this is a separate issue.

Ok, thanks, Jamie.

Robert please ignore that my comment then.

Comment 51

[James Teh [:Jamie]]

Okay. Some debugging reveals that AccessibleObjectFromEvent() always returns the accessible for the root document when requesting an accessible in a sub-document. For example, if a link in a sub-document gets focus, it fires an event specifying the hwnd of the document and the unique ID of the link. However, when AccessibleObjectFromEvent() is called with those parameters, it returns the accessible for the root document instead of the accessible for the link. This causes a great deal of nastiness.

Note that this only occurs for sub-documents. Documents without frames/iframes are fine, as might be expected.

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #39)
> My guess is that we're dispatching the query content events at the top level
> document (since it's the only document with a widget), and
> nsContentEventHandler is only looking at that document. It needs to look at the
> document that has focus. Perhaps the right thing to do is to forward query
> content events to the focused element the same way we do for IME and key
> events, here?
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6099

No, you need to change here:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5914

> 5914   // key and IME events must be targeted at the presshell for the focused frame
> 5915   if (!sDontRetargetEvents &&
> 5916       (NS_IS_KEY_EVENT(aEvent) || NS_IS_IME_EVENT(aEvent) ||
> +           NS_IS_QUERY_CONTENT_EVENT(aEvent) || NS_IS_SELECTION_EVENT(aEvent) ||
> 5917        NS_IS_CONTEXT_MENU_KEY(aEvent))) {
> 5918     nsIFocusManager* fm = nsFocusManager::GetFocusManager();
> 5919     if (!fm)
> 5920       return NS_ERROR_FAILURE;

This works fine. However, this means that we cannot use the content events in deactivated window. But it's ok for now if you don't have better fix.

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Great, thanks!

Comment 54

[James Teh [:Jamie]]

(In reply to comment #51)
> Some debugging reveals that AccessibleObjectFromEvent() always returns
> the accessible for the root document when requesting an accessible in a
> sub-document.
Further details:
AccessibleObjectFromEvent() sends a WM_GETOBJECT message, which returns the root accessible for the window. Before this patch, the root accessible was the accessible for the sub-document, but now, it is the accessible for the top document (since there is only one window/hwnd).
AccessibleObjectFromEvent() then calls get_accChild() on this accessible, passing it the child ID received in the win event. This child ID is the unique ID from Gecko.
On the Gecko side, this calls NSDocAccessibleWrap::get_accChild(), which searches a cache for an accessible with the specified unique ID.
Unfortunately, with this patch, this gets called on the accessible for the top document. However, the unique ID supplied is for an accessible in a sub-document, so it will not be found in the top document's cache.
Fail!

Comment 55

[alexander :surkov (:asurkov)]

Attachment: part 3.2 fix accessibility (should be applied with part 3)

Comment 56

[alexander :surkov (:asurkov)]

Robert could you do one more try build including my patch?

Comment 57

[Atsushi Sakai]

Attachment: screenshot

On Mac, the position of IM's candidate window is wrong with Kotoeri.

Comment 58

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #57)
> Created an attachment (id=386534) [details]
> screenshot
> 
> On Mac, the position of IM's candidate window is wrong with Kotoeri.

Isn't it fixed by the code of comment 52?

Comment 59

[Neil Deakin]

Comment on attachment 386182 [details] [diff] [review]
Part 7: fix nsXULPopupManager's saved mouse coordinates

               // no widget, so just use the client point if available
+              // XXXroc when does this happen?

For popupshowing and other popup events, which use nsMouseEvent. 

>+              // XXX this doesn't handle IFRAMEs in transforms
>+              nsPoint offsetToRoot =
>+                presShell->FrameManager()->GetRootFrame()->GetOffsetTo(rootFrame);

I got a bit lost here because the term 'root' is used to refer to two different things. It's confusing that we're trying to getting the offset from GetRootFrame() to rootFrame. Can you use better names for each?

Comment 60

[Atsushi Sakai]

Comment on attachment 386534 [details]
screenshot

(In reply to comment #58)
> > On Mac, the position of IM's candidate window is wrong with Kotoeri.
> 
> Isn't it fixed by the code of comment 52?

Right. No problem with comment 52.

Comment 61

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #60)
> Right. No problem with comment 52.

Great, thank you for your testing.

Comment 62

[Robert O'Callahan (:roc) (email my personal email if necessary)]

New try builds here:
https://build.mozilla.org/tryserver-builds/[e-mail]-try-be5734263a2f/
All pending review comments in this bug and bug 339548 have been addressed. I've added Alexander Surkov's patch. Latest patches uploaded here:
http://hg.mozilla.org/users/rocallahan_mozilla.com/compositor-patches/

Comment 63

[Atsushi Sakai]

I found a problem, even with the new try build.

Steps to reproduce:
1. Use Windows XP.
2. Go to Control Panel -> Date, Time, Language, and Regional Options -> Regional and Language Options -> Languages.
3. Enable Install files for East Asian languages in Supplemental language support, and click Apply.
4. Click Details in Text services and input languages (in the same tab as 2.).
5. Click Add in Installed services.
6. Select Japanese as Input language and select Microsoft Natural Input 2002 ver.8.1 as Keyboard layout/IME.
7. Click OK, OK, OK
8. Start Firefox.
9. Change intl.enable_tsf_support to true and restart Firefox.
10. Open data:text/html,<iframe src="data:text/html,<input>"></iframe>
11. Move focus to the <input>.
12. Choose JP (Japanese) as language in the Lnaguage Bar. (See http://support.microsoft.com/kb/306993 if you don't know)
13. Choose Hiragana as Input Mode in the Language Bar.
14. Press "a" key. Underlined "あ" will appear.
15. Press Tab key.

Actual result:
The focus moves, but "あ" is still underlined.

Expected result:
"あ" should be committed, i.e. not underlined.
Or, focus should not move at this time.
I'm not sure which behavior is correct.

Comment 64

[Robert O'Callahan (:roc) (email my personal email if necessary)]

TSF is not enabled by default yet due to bugs. I would prefer to not block on TSF issues.

Comment 65

[Marco Zehe (:MarcoZ)]

This new try build crashes when I log into GMail (after login, when the Inbox should appear). This is with JAWS 10 running.

Comment 66

[alexander :surkov (:asurkov)]

(In reply to comment #65)
> This new try build crashes when I log into GMail (after login, when the Inbox
> should appear). This is with JAWS 10 running.

It might be my bad, I'll debug.

Comment 67

[alexander :surkov (:asurkov)]

(In reply to comment #66)
> (In reply to comment #65)
> > This new try build crashes when I log into GMail (after login, when the Inbox
> > should appear). This is with JAWS 10 running.
> 
> It might be my bad, I'll debug.

I don't get crash when my patch is applied only. Robert is there a easy way to apply all your patches and compile own build?

Comment 68

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes. I'm not sure of the best way to do it, but an easy way would be something like this:

hg clone https://hg.mozilla.org/mozilla-central
hg clone http://hg.mozilla.org/users/rocallahan_mozilla.com/compositor-patches
cd mozilla-central
hg qinit
hg update -C -r 1c6ec6110a0f
cp ../compositor-patches/* .hg/patches
mkdir layout/base/tests/chrome
hg qpush -a

That mkdir is to work around a Mercurial bug that's affect the chromify-tests patch.

You could do this in an existing mozilla-central checkout if you're not already using MQ in it.

1c6ec6110a0f is the changeset that my patches currently apply cleanly to. That's trunk as of a couple of days ago.

Comment 69

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #64)
> TSF is not enabled by default yet due to bugs. I would prefer to not block on
> TSF issues.

Yes. And the issue will be gone by the next patch of bug 480708.

Comment 70

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 386176 [details] [diff] [review]
Part 2: Create nsIViewManager::GetRootWidget

r=bzbarsky

Comment 71

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 386195 [details] [diff] [review]
Part 13: remove nsIViewManager::GetWidget

r=bzbarsky, assuming the plan is to add callers of this new GetWidget.

Comment 72

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 386193 [details] [diff] [review]
Part 11: remove nsViewManager calls to nsViewManager::GetWidget

r=bzbarsky, though the nsViewManager.cpp change here looks like it should be in an earlier patch on this bug or something.

Comment 73

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 386194 [details] [diff] [review]
Part 12: Teach nsDocumentViewer and nsDocShell to deal with not having a widget

>+++ b/layout/base/nsDocumentViewer.cpp
> DocumentViewerImpl::InitInternal(nsIWidget* aParentWidget,
>   if (aDoCreation) {
>+    nsresult rv = CreateDeviceContext(containerView);

This looks like it adds an extra call to CreateDeviceContext that we didn't use to have before when coming from SetPageMode.  Is that desirable?  I guess it's ok at the very least; CreateDeviceContext will just create a new one and be done with it...

Also, the documentation for InitInternal in the class decl needs fixing, looks like.

> DocumentViewerImpl::FindContainerView()

I really wish we had a better way to do what you need here in the !mParentWidget && mContainer case...

Maybe we could just improve the relevant APIs to stop shoehorning everything into nsIBaseWindow and just pass in the parent view directly...  We can worry about this later.  Please do file a followup bug to clean this up, though?

>+    nsCOMPtr<nsIDocShell> docShell(do_QueryReferent(mContainer));

Might as well query into an nsIDocShellTreeItem here.  You need one later on anyway, and you never need an nsIDocShell for mContainer.

>+DocumentViewerImpl::CreateDeviceContext(nsIView* aContainerView)
>   if (doc) {
>-    NS_ASSERTION(!aWidget, "Shouldn't have a widget here");

Should still be able to assert !aContainerView, no?

>+++ b/widget/public/nsIBaseWindow.idl
>+	It is possible to pass null for both parentNativeWindow and parentWidget,
>+	but only docshells support this.

Yeah, I was going to make nsWebBrowser do that too.... ;)

r=bzbarsky with those nits picked.

Comment 74

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes to all those comments.

Comment 75

[alexander :surkov (:asurkov)]

Attachment: part 3.2 fix accessibility (should be applied with part 3, version 2)

It doesn't crash and read gmail.com content for me.

Comment 76

[alexander :surkov (:asurkov)]

Comment on attachment 386865 [details] [diff] [review]
part 3.2 fix accessibility (should be applied with part 3, version 2)

sorry for the spam, this version works incorrectly, I tested wrong build.

Comment 77

[Atsushi Sakai]

On Mac OS X 10.5.7, crashes several times a day.
Though I don't yet find reliable steps to reproduce.

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000048
0x02860fe1 in nsCOMPtr<nsIWidget>::get (this=0x48) at nsCOMPtr.h:777
777	          return reinterpret_cast<T*>(mRawPtr);
(gdb) bt
#0  0x02860fe1 in nsCOMPtr<nsIWidget>::get (this=0x48) at nsCOMPtr.h:777
#1  0x02860ff7 in nsCOMPtr<nsIWidget>::operator nsIWidget* (this=0x48) at nsCOMPtr.h:790
#2  0x02b8c5d2 in nsPluginInstanceOwner::FixUpPluginWindow (this=0x0, inPaintState=1) at /builds/mozilla-central/mozilla/layout/generic/nsObjectFrame.cpp:4972
#3  0x02b8c9fa in nsObjectFrame::DidSetWidgetGeometry (this=0x249dc17c) at /builds/mozilla-central/mozilla/layout/generic/nsObjectFrame.cpp:1189
#4  0x02b0e60f in PluginDidSetGeometryEnumerator (aEntry=0x1e7e1380, userArg=0x0) at /builds/mozilla-central/mozilla/layout/base/nsPresContext.cpp:2214
#5  0x02b14763 in nsTHashtable<nsPtrHashKey<nsObjectFrame> >::s_EnumStub (table=0x5e60a40, entry=0x1e7e1380, number=0, arg=0xbfffd858) at nsTHashtable.h:420
#6  0x035b1c03 in PL_DHashTableEnumerate (table=0x5e60a40, etor=0x2b14746 <nsTHashtable<nsPtrHashKey<nsObjectFrame> >::s_EnumStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*)>, arg=0xbfffd858) at pldhash.c:754
#7  0x02b13c0d in nsTHashtable<nsPtrHashKey<nsObjectFrame> >::EnumerateEntries (this=0x5e60a40, enumFunc=0x2b0e5f0 <PluginDidSetGeometryEnumerator>, userArg=0x0) at nsTHashtable.h:241
#8  0x02b0eb34 in nsRootPresContext::DidApplyPluginGeometryUpdates (this=0x5e60800) at /builds/mozilla-central/mozilla/layout/base/nsPresContext.cpp:2221
#9  0x02b134ac in nsRootPresContext::UpdatePluginGeometry (this=0x5e60800, aChangedRoot=0x5e623e0) at /builds/mozilla-central/mozilla/layout/base/nsPresContext.cpp:2207
#10 0x02acfc80 in nsCSSFrameConstructor::ProcessRestyledFrames (this=0x1e7e3d10, aChangeList=@0xbfffd978) at /builds/mozilla-central/mozilla/layout/base/nsCSSFrameConstructor.cpp:7797
#11 0x02ad01c6 in nsCSSFrameConstructor::ProcessOneRestyle (this=0x1e7e3d10, aContent=0x1e9ae9e0, aRestyleHint=0, aChangeHint=nsChangeHint_RepaintFrame) at /builds/mozilla-central/mozilla/layout/base/nsCSSFrameConstructor.cpp:11617
#12 0x02ad1862 in nsCSSFrameConstructor::ProcessPendingRestyles (this=0x1e7e3d10) at /builds/mozilla-central/mozilla/layout/base/nsCSSFrameConstructor.cpp:11718
#13 0x02b21c0f in PresShell::FlushPendingNotifications (this=0x5e68800, aType=Flush_Style) at /builds/mozilla-central/mozilla/layout/base/nsPresShell.cpp:4833
#14 0x02abfed8 in nsCSSFrameConstructor::RestyleEvent::Run (this=0x22944570) at /builds/mozilla-central/mozilla/layout/base/nsCSSFrameConstructor.cpp:11802
#15 0x035f9a07 in nsThread::ProcessNextEvent (this=0x501d80, mayWait=0, result=0xbfffe17c) at /builds/mozilla-central/mozilla/xpcom/threads/nsThread.cpp:527
#16 0x035b8cc7 in NS_ProcessPendingEvents_P (thread=0x501d80, timeout=20) at nsThreadUtils.cpp:180
#17 0x0355c0fb in nsBaseAppShell::NativeEventCallback (this=0x533c20) at /builds/mozilla-central/mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:121
#18 0x03520bee in nsAppShell::ProcessGeckoEvents (aInfo=0x533c20) at /builds/mozilla-central/mozilla/widget/src/cocoa/nsAppShell.mm:413
#19 0x92b1c595 in CFRunLoopRunSpecific ()
#20 0x92b1cc78 in CFRunLoopRunInMode ()
#21 0x96a6028c in RunCurrentEventLoopInMode ()
#22 0x96a5ffde in ReceiveNextEventCommon ()
#23 0x96a5ff19 in BlockUntilNextEventMatchingListInMode ()
#24 0x939fed0d in _DPSNextEvent ()
#25 0x939fe5c0 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#26 0x939f75fb in -[NSApplication run] ()
#27 0x0351f02d in nsAppShell::Run (this=0x533c20) at /builds/mozilla-central/mozilla/widget/src/cocoa/nsAppShell.mm:766
#28 0x03352c9a in nsAppStartup::Run (this=0x552d80) at /builds/mozilla-central/mozilla/toolkit/components/startup/src/nsAppStartup.cpp:193
#29 0x02852d36 in XRE_main (argc=1, argv=0xbffff78c, aAppData=0x50ed70) at /builds/mozilla-central/mozilla/toolkit/xre/nsAppRunner.cpp:3386
#30 0x00002489 in main (argc=1, argv=0xbffff78c) at /builds/mozilla-central/mozilla/browser/app/nsBrowserApp.cpp:156

Comment 78

[Atsushi Sakai]

Steps to reproduce for previous comment:
1. Go to http://b.hatena.ne.jp/entry/www.nicovideo.jp/watch/sm101534
2. Go to another page in the same tab.
3. Click Back button and wait for few seconds.
4. Click Forward button and wait for few seconds.
5. Repeat 3. and 4. several times.

Comment 79

[Josh Aas]

Comment on attachment 386202 [details] [diff] [review]
Part 15: make scrollframes windowless

I only looked at the mac code, looks good to me. Nice to see nsChildView::Scroll cleaned up.

Comment 80

[Atsushi Sakai]

(In reply to comment #63)
> I found a problem, even with the new try build.
> 
> Steps to reproduce:
> 1. Use Windows XP.

The same problem can be reproduced on Windows 7 RC + Microsoft IME if TSF is enabled.

Comment 81

[Atsushi Sakai]

(In reply to comment #77)
> On Mac OS X 10.5.7, crashes several times a day.

> #2  0x02b8c5d2 in nsPluginInstanceOwner::FixUpPluginWindow (this=0x0,
> inPaintState=1) at
> /builds/mozilla-central/mozilla/layout/generic/nsObjectFrame.cpp:4972
> #3  0x02b8c9fa in nsObjectFrame::DidSetWidgetGeometry (this=0x249dc17c) at
> /builds/mozilla-central/mozilla/layout/generic/nsObjectFrame.cpp:1189

I guess Part 10.5 of bug 339548 triggers this crash.

Comment 82

[alexander :surkov (:asurkov)]

Attachment: part 3.2 fix accessibility (should be applied with part 3, version 3)

Ok, it makes gmail.com to work with JAWS (look for accessible through all cached documents). Though I really don't understand deeply all related code. So I haven't idea why previous version of the patch don't work (where I search through the document and its subdocuments caches). Also we have some a11y related code in Windows.cpp and I'm not sure if we should change something there. I'll put the link on try build a bit later.

Comment 83

[alexander :surkov (:asurkov)]

try build with Roc's patches and my patch is available here https://build.mozilla.org/tryserver-builds/[e-mail]-try-aac7f5d1075d/

Comment 84

[Marco Zehe (:MarcoZ)]

This latest try server build no longer crashes with JAWS 10 running and logging into Gmail.

But NVDA is still completely broken in GMail. The virtual buffer is blank, there is no content at all. On pages without iframes everything's fine. But with this, we're completely breaking NVDA in GMail and other places.

Comment 85

[Karl Tomlinson (:karlt)]

Comment on attachment 386202 [details] [diff] [review]
Part 15: make scrollframes windowless

+     * be performed simultaneously with the scrolling, as far as possible,658

Accidental paste, it seems.

   // Note that child widgets may be scrolled by the native widget scrolling,
   // so don't update their positions

Does this comment need updating?

+     * Scroll this widget and (as simultaneously as possible) modify
+     * child widgets.

"Scroll a rectangle in this widget"?
"modify the specified child widgets"?

+    GdkDrawable* drawable = GDK_DRAWABLE(mDrawingarea->inner_window);
+    GdkGC* gc = gdk_gc_new(drawable);
+    gdk_draw_drawable(drawable, gc, drawable,
+                      aSource.x, aSource.y,
+                      aSource.x + aDelta.x, aSource.y + aDelta.y,
+                      aSource.width, aSource.height);
+    g_object_unref(gc);

Don't you want graphics_exposures set here?
Xlib GCs default graphics_exposures to True but GdkGCs default to False.

Have you considered gdk_window_move_region here?
The main advantage would be that it ensures that expose events not yet
processed get translated appropriately.
It also does the necessary adjustment of the update area.
It invalidates the portion of the source not in the destination
(which we do anyway) but that doesn't look like it'll be a problem.

+    nsIntRect affectedArea;
+    affectedArea.UnionRect(aSource, aSource + aDelta);

This is not used.

+        if (configuration->mClipRegion.IsEmpty() ||
+            configuration->mBounds != w->mBounds) {
+            w->Show(PR_FALSE);

Setting an empty clip region would seem less invasive then effecting an unmap.
I wouldn't know for sure without trying, but it might also mean that the code
to set the configurations could be shared with nsWindow::ConfigureChildren.

Comment 86

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #85)
>    // Note that child widgets may be scrolled by the native widget scrolling,
>    // so don't update their positions
> 
> Does this comment need updating?

I just removed it.

> +     * Scroll this widget and (as simultaneously as possible) modify
> +     * child widgets.
> 
> "Scroll a rectangle in this widget"?
> "modify the specified child widgets"?

OK

> Setting an empty clip region would seem less invasive then effecting an unmap.
> I wouldn't know for sure without trying, but it might also mean that the code
> to set the configurations could be shared with nsWindow::ConfigureChildren.

Using gdk_window_move_region and setting an empty clip region to hide the widgets causes major plugin flickering when scrolling. I'll try to figure out why.

Comment 87

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #86)
> Using gdk_window_move_region and setting an empty clip region to hide the
> widgets causes major plugin flickering when scrolling. I'll try to figure out
> why.

It appears that setting the clip region to empty causes a gray area to be eagerly painted where the plugin was. Using gdk_window_move_region is probably going to be OK.

Comment 88

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 7 v2: fix nsXULPopupManager's saved mouse coordinates

I hope this is clearer

Comment 89

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 15 v2: make scrollframes windowless

Karl's comments addressed.

Comment 90

[Robert O'Callahan (:roc) (email my personal email if necessary)]

New tryserver builds: https://build.mozilla.org/tryserver-builds/[e-mail]-try-54ed3b16498d/

Comment 91

[Marco Zehe (:MarcoZ)]

These try-server builds exhibit the same problem with NVDA as the previous ones: The virtual buffer still stays blank. JAWS continues to work fine.

Comment 92

[Jim Mathies [:jimm]]

Comment on attachment 388196 [details] [diff] [review]
Part 15 v2: make scrollframes windowless

The win32 scrolling code looks good. Running this for the weekend didn't turn anything up.

It'd be great if we could work out any TSF issues as we really wanted to get that enabled by default for better win7 touch input support.

Comment 93

[Atsushi Sakai]

The new tryserver build does not crash with Comment #78.
Thanks for fixing it.

Comment 94

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #91)
> These try-server builds exhibit the same problem with NVDA as the previous
> ones: The virtual buffer still stays blank. JAWS continues to work fine.

Right, I used the same patch from Alexander as last time. We need a new patch from Alexander to fix NVDA. (Unless we're triggering an NVDA issue now?)

Comment 95

[James Teh [:Jamie]]

(In reply to comment #91)
> These try-server builds exhibit the same problem with NVDA as the previous
> ones: The virtual buffer still stays blank.
Unfortunately, it seems there is another problem. :( IAccessible2::get_windowHandle is failing on sub-documents, returning 0 instead of the actual window handle. NVDA needs this to render buffers correctly.

Comment 96

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #95)
> IAccessible2::get_windowHandle is failing on sub-documents, returning 0 instead
> of the actual window handle.

One of the major goals of the work here is stop sub-documents from having actual native windows. So instead of 0, should we be returning the HWND of the nearest enclosing document that actually has a native window?

Comment 97

[alexander :surkov (:asurkov)]

(In reply to comment #96)
> (In reply to comment #95)
> > IAccessible2::get_windowHandle is failing on sub-documents, returning 0 instead
> > of the actual window handle.
> 
> One of the major goals of the work here is stop sub-documents from having
> actual native windows. So instead of 0, should we be returning the HWND of the
> nearest enclosing document that actually has a native window?

That should be right I think.

Comment 98

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Assuming that's what you want, I have a fix and I'll make try-server builds later today.

Comment 99

[James Teh [:Jamie]]

(In reply to comment #96)
> One of the major goals of the work here is stop sub-documents from having
> actual native windows.
Understood.
> So instead of 0, should we be returning the HWND of the
> nearest enclosing document that actually has a native window?
Yes please. :) As I understand it, get_windowHandle will then return the same hwnd for the top document and sub-documents.

Comment 100

[Robert O'Callahan (:roc) (email my personal email if necessary)]

New try builds, with that accessibility fix and updated to latest trunk:
https://build.mozilla.org/tryserver-builds/[e-mail]-try-57050fb709f6/

Comment 101

[Marco Zehe (:MarcoZ)]

The Windows builds haven't appeared as of 1:15 AM PDT. Did the build fail?

Comment 102

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I failed, by giving you the wrong link.
https://build.mozilla.org/tryserver-builds/[e-mail]-try-cc17fe5d2d61/

Comment 103

[Marco Zehe (:MarcoZ)]

I can confirm that this try-server build works with both JAWS and NVDA in GMail.

Comment 104

[Marco Zehe (:MarcoZ)]

Also confirmed with this try-server build that Window-Eyes, SuperNova and System Access still work. All's good on supporting assistive technologies on Windows!

Comment 105

[Michael Curran]

(In reply to comment #104)
> Also confirmed with this try-server build that Window-Eyes, SuperNova and
> System Access still work. All's good on supporting assistive technologies on
> Windows!
Sorry, I can confirm that NVDA now shows the content of frames and iframes. However, there is yet another issue with this try build and NVDA:
NVDA no longer will pick up new document loads in frames and iframes. E.g. http://xahlee.org/js/frame/0.html

Clicking  'b' or 'c' links in the first (left) frame load a new document in that frame, yet NVDA does not reflect this in its virtualBuffer. This works with a standard nightly of Firefox, but not this try build. 
I have also got Marco to confirm this.
Jamie and I will try and provide some more info on this tomorrow as we investigate why NVDA is failing in this situation.

Comment 106

[James Teh [:Jamie]]

(In reply to comment #105)
> NVDA no longer will pick up new document loads in frames and iframes.
This problem is caused by a check in our code relating to a work around for bug 420845. Because a reorder event is not fired on sub-frames when a new document is loaded into them, we have to use the state change on the new document. Normally, we ignore events for nodes which don't exist in our buffer (which is the case for this new document). However, due to bug 420845, we have a special exception for a state change on a node which has a different window handle to the root document. This check relies on an assumption (different window handle for sub-documents) which is changed by these patches, hence the breakage.

I think I have a solution on our side. However, bug 420845 probably should still be fixed at some point.

Comment 107

[alexander :surkov (:asurkov)]

James, if we'll fix the bug 420845 then will current NVDA version work? It's worth not to force NVDA users to upgrade NVDA once they upgraded firefox.

Comment 108

[James Teh [:Jamie]]

(In reply to comment #107)
> James, if we'll fix the bug 420845 then will current NVDA version work?
Yes. Note that I've worked around the issue in our code now anyway, but that obviously won't affect people running earlier releases of NVDA.

Comment 109

[Karl Tomlinson (:karlt)]

Comment on attachment 388196 [details] [diff] [review]
Part 15 v2: make scrollframes windowless

r+=me on the GTK changes.

try-cc17fe5d2d61 seemed to scroll plugins nicely on my system with and without kompmgr running.

We'll need this soon as guffaw scrolling has been removed from git GTK+ with the landing of client-side windows (probably because child windows with real X windows are expected to be rare):
http://git.gnome.org/cgit/gtk+/commit/?id=eabac453e652d5aa2e535d957057f9c84803eea9

Comment 110

[alexander :surkov (:asurkov)]

Robert, could you publish new try build updated to trunk because bug 420845 was fixed?

Comment 111

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Updated builds here:
https://build.mozilla.org/tryserver-builds/[e-mail]-try-3021cd75023e/
These are updated to today's trunk and contain the fix for bug 420845. They also contain the latest patches for bug 339548 and bug 505184.

Comment 112

[Markus Stange [:mstange]]

With that build I noticed that scrolling in the awesomebar dropdown doesn't work correctly on Mac. The newly-visible parts are drawn correctly but the rest of the content isn't moved.
This bug also exists in the build from comment 34.

Comment 113

[Marco Zehe (:MarcoZ)]

This latest try server build is fine with both JAWS and NVDA in both pages and framesets. The example Mick and Jamie mentioned also works fine with the same build of NVDA that a few days ago would not pick up the changes.

From my end this is good!

Comment 114

[alexander :surkov (:asurkov)]

Robert, please upload new a11y patch with your additional HWND fix from comment #98.

Comment 115

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 387602 [details] [diff] [review]
part 3.2 fix accessibility (should be applied with part 3, version 3) 

r=me thanks (I didn't try the patch out). Some minor things:

> {
>   *aXPAccessible = nsnull;
>-  if (!mWeakShell)
>-    return; // This document has been shut down

>+  if (IsDefunct())
>     return;

Note we don't currently check mWeakShell in IsDefunct but we do check mDocument which should be sufficient? (I don't have time to check this right now)

> 
>-STDMETHODIMP nsDocAccessibleWrap::get_accChild( 
>-      /* [in] */ VARIANT varChild,
>-      /* [retval][out] */ IDispatch __RPC_FAR *__RPC_FAR *ppdispChild)
>+STDMETHODIMP
>+nsDocAccessibleWrap::get_accChild(VARIANT aVarChild,
>+                                  IDispatch __RPC_FAR *__RPC_FAR *aPPDispChild)

nit: I think the name ppdispChild is more recognizable for COM stuff, and I'm not sure about capitalizing the hungarian notation. My preference would be to leave the name as is here.

>+
>+struct nsSearchAccessibleInCacheArg
>+{
>+  nsCOMPtr<nsIAccessNode> mAccessNode;
>+  void *mUniqueID;
>+};
>+
>+static PLDHashOperator
>+SearchAccessibleInCache(const void* aKey, nsIAccessNode* aAccessNode,
>+                        void* aUserArg)
>+{
>+  nsCOMPtr<nsIAccessibleDocument> docAccessible(do_QueryInterface(aAccessNode));
>+  NS_ASSERTION(docAccessible,
>+               "No doc accessible for the object in doc accessible cache!");

When can this happen? If never, fine :)  If there are known cases we should invalidate the cache (and possibly bail out).


>+
>+  // nsDocAccessibleWrap
>+
>+  /**
>+   * Search the accessible by the given child ID though cached documents.

How about: "Find the document accessible that contains the given child ID." ?

>+  static void GetXPAccessibleForChildID(const VARIANT& aVarChild,

The pldhash stuff looks right but I've not used it before.

Comment 116

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3 v2: Accessibility fixes

Comment 117

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 15 v3: fix Mac scrolling issue

Need to implement ::Scroll on nsCocoaWindow, forwarding to mPopupContentView. This fixes the awesomebar scrolling bug that Markus mentioned.

Comment 118

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 389557 [details] [diff] [review]
Part 15 v3: fix Mac scrolling issue

Still need dbaron review on the layout/view changes

Comment 119

[alexander :surkov (:asurkov)]

Comment on attachment 389544 [details] [diff] [review]
Part 3 v2: Accessibility fixes

r=me

Comment 120

[alexander :surkov (:asurkov)]

(In reply to comment #115)
> (From update of attachment 387602 [details] [diff] [review])
> r=me thanks (I didn't try the patch out). Some minor things:
> 
> > {
> >   *aXPAccessible = nsnull;
> >-  if (!mWeakShell)
> >-    return; // This document has been shut down
> 
> >+  if (IsDefunct())
> >     return;

> Note we don't currently check mWeakShell in IsDefunct but we do check mDocument
> which should be sufficient? (I don't have time to check this right now)

nsDocAccessible::IsDefunct() calls IsDefunct from base class, so we check mWeakShell.

> nit: I think the name ppdispChild is more recognizable for COM stuff, and I'm
> not sure about capitalizing the hungarian notation. My preference would be to
> leave the name as is here.
 
ok

> >+  nsCOMPtr<nsIAccessibleDocument> docAccessible(do_QueryInterface(aAccessNode));
> >+  NS_ASSERTION(docAccessible,
> >+               "No doc accessible for the object in doc accessible cache!");
> 
> When can this happen? If never, fine :)  If there are known cases we should
> invalidate the cache (and possibly bail out).

never I think

> >+   * Search the accessible by the given child ID though cached documents.
> 
> How about: "Find the document accessible that contains the given child ID." ?

It's not correct, I changed on "Find an accessible by the given child ID in cached documents."

Comment 121

[Josh Aas]

Comment on attachment 389557 [details] [diff] [review]
Part 15 v3: fix Mac scrolling issue

>diff --git a/widget/src/cocoa/nsCocoaWindow.mm b/widget/src/cocoa/nsCocoaWindow.mm
>--- a/widget/src/cocoa/nsCocoaWindow.mm
>+++ b/widget/src/cocoa/nsCocoaWindow.mm
>@@ -817,21 +817,30 @@ NS_IMETHODIMP nsCocoaWindow::Show(PRBool
>   return NS_OK;
> 
>   NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
> }
> 
> nsresult
> nsCocoaWindow::ConfigureChildren(const nsTArray<Configuration>& aConfigurations)
> {
>-  for (PRUint32 i = 0; i < aConfigurations.Length(); ++i) {
>-    nsChildView::ApplyConfiguration(this, aConfigurations[i], PR_TRUE);
>+  if (mPopupContentView) {
>+    mPopupContentView->ConfigureChildren(aConfigurations);
>   }
>   return NS_OK;
>-} 
>+}

I don't fully understand this change, can you explain?

Comment 122

[alexander :surkov (:asurkov)]

Attachment: part 3.2 v4 (should applied within part 3 v2)

Comment 123

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #121)
> I don't fully understand this change, can you explain?

nsCocoaWindow can't have direct plugin children, but it can have a mPopupContentView and we have to delegate scroll and ConfigureChildren calls to it.

Comment 124

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 386180 [details] [diff] [review]
Part 6: fix GetScreenRect to handle appunits-per-dev-pixel changes across document boundaries where there's no widget

r=dbaron.

I suppose you'll have to rewrite nsLayoutUtils::GetCrossDocParentFrame when you do view removal.

We probably want to have some automated tests for this (the things that use this function, in cases where the inner document is zoomed... which I presume is what can cause this case), though.

Comment 125

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 386191 [details] [diff] [review]
Part 9: fix context menu event coordinates

>+  mPresContext->RootPresContext()->PresShell()->GetViewManager()->aEvent
>+    GetRootWidget(getter_AddRefs(aEvent->widget));

r=dbaron asuming the "aEvent" at the end of the first line above is extraneous, and that you've actually tested this patch without that.

(Do we have tests that cover this?)

Comment 126

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #124)
> We probably want to have some automated tests for this (the things that use
> this function, in cases where the inner document is zoomed... which I presume
> is what can cause this case), though.

Some tests trigger this, actually, that's how I found it. Right now at 6am I don't remember which ones

(In reply to comment #125)
> r=dbaron asuming the "aEvent" at the end of the first line above is extraneous,
> and that you've actually tested this patch without that.
> 
> (Do we have tests that cover this?)

Yes, we do.

Comment 127

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 389557 [details] [diff] [review]
Part 15 v3: fix Mac scrolling issue

Please document in nsIScrollPositionListener.h that aConfigurations is
an array to be appended to by the implementation.  (Is there a good
reason it's a pointer rather than a reference, both here and in your
patch named "plugin-shaping"?)

Is there a useful way to share the code in nsTreeBodyFrame?

Why is passing an empty configurations array OK in nsTreeBodyFrame?  Is
it because things in trees never claim to be opaque?  Could you document
the reason?

In nsScrollPortView::Scroll... is the second parameter in the call to
AdjustChildWidgets still right in general?  The case you removed
(!scrollWidget) had a different one that looked a little more general to
me than the one in the case you're keeping, but I'm really not sure.

>+      nsIntRect destScroll = (biggestRect + nearestWidgetOffset).ToInsidePixels(aP2A);

Why is this ToInsidePixels rather than ToNearestPixels?  Couldn't this 
result in repainting fringes around things that you're blitting?


I didn't look too closely at the widget changes; I assume the other
reviewers looked at them better than I could.

r=dbaron

Comment 128

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #127)
> (From update of attachment 389557 [details] [diff] [review])
> Please document in nsIScrollPositionListener.h that aConfigurations is
> an array to be appended to by the implementation.  (Is there a good
> reason it's a pointer rather than a reference, both here and in your
> patch named "plugin-shaping"?)

No, other than I prefer pointers for out or in-out parameters because it's more obvious in the callers that the parameter can be modified.

> Is there a useful way to share the code in nsTreeBodyFrame?

Yes, I'll do that.

> Why is passing an empty configurations array OK in nsTreeBodyFrame?  Is
> it because things in trees never claim to be opaque?  Could you document
> the reason?

It's because a plugin would never have a tree widget as its parent widget. I'll document that.

> In nsScrollPortView::Scroll... is the second parameter in the call to
> AdjustChildWidgets still right in general?  The case you removed
> (!scrollWidget) had a different one that looked a little more general to
> me than the one in the case you're keeping, but I'm really not sure.

We're passing the same value: the offset result from GetNearestWidget. That's what we want.

> >+      nsIntRect destScroll = (biggestRect + nearestWidgetOffset).ToInsidePixels(aP2A);
> 
> Why is this ToInsidePixels rather than ToNearestPixels?  Couldn't this 
> result in repainting fringes around things that you're blitting?

Yes it could. Generally scrolls should be pixel-aligned, hopefully. But when they're not, I don't want to scroll something that could include anti-aliased fragments of something that's not moving.

Comment 129

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> > Is there a useful way to share the code in nsTreeBodyFrame?
> 
> Yes, I'll do that.

Actually there isn't a great way to do that, since the horizontal and vertical scrolling cases require different code; unifying them would require more complex code involving regions. So I'll pass on that.

Comment 130

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I landed everything. Things are mostly good except we seem to have a Tdhtml regression on Linux. I filed bug 505665 for that instead of backing out because a) backing all this out will be extremely painful b) it would put us at schedule risk c) Tdhtml is unreliable d) only Linux affected and e) we can now get rid of mozdrawingarea on GTK which will probably win back.

Comment 131

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/dfa7ab1fe97c
http://hg.mozilla.org/mozilla-central/rev/5eb5d45227a1
http://hg.mozilla.org/mozilla-central/rev/060113a3eb2c
http://hg.mozilla.org/mozilla-central/rev/e279f79fd1b9
http://hg.mozilla.org/mozilla-central/rev/1cab5ea84f6b
http://hg.mozilla.org/mozilla-central/rev/0ea768ecfd77
http://hg.mozilla.org/mozilla-central/rev/c401fb5c6fb3
http://hg.mozilla.org/mozilla-central/rev/447743d74ca3
http://hg.mozilla.org/mozilla-central/rev/41b4d9d1cd92
http://hg.mozilla.org/mozilla-central/rev/62246e366eb5
http://hg.mozilla.org/mozilla-central/rev/48655af83e5f
http://hg.mozilla.org/mozilla-central/rev/9bc2931f032b
http://hg.mozilla.org/mozilla-central/rev/9b8e2261d5fa
http://hg.mozilla.org/mozilla-central/rev/b01f80d5166a
http://hg.mozilla.org/mozilla-central/rev/026403392281
http://hg.mozilla.org/mozilla-central/rev/ff084019260e

Comment 132

[Sander]

I've walked through the entire dependency tree and marked bugs as fixed where possible. Remaining are the following:
bug 215055 - fixed.
  bug 470358 - unable to reproduce as stated (Linux? The testcase to me appears to be the same as bug 215055, which is fixed.)
    bug 444238 - unable to reproduce: original source doesn't exist anymore.
  bug 333994 - unable to reproduce: testcase depends on original source which doesn't exist anymore.
bug 242917 - unable to reproduce (mac?)
bug 266582 - unable to reproduce (linux xft only) + original source gone.
bug 303036 - unable to reproduce (no printer)
bug 337801 + entire tree - old performance tracker dependency mess (bz kept bug 118933 open a year ago; might be really wfm now?) + some other bugs not expected to be fixed
bug 361354 + tree - other bugs not expected to be fixed
bug 443766 - hard to reproduce; possibly not fixed?
bug 449734 - not expected to be fixed
bug 469007 - unable to reproduce (mac)

Comment 133

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thanks

Comment 134

[Barry Marshall]

Could this patch be causing Bug 507222 -  Thinkpad Trackpoint middle button mouse scroll does not scroll window up and down ?  Bustage happened in the nightly after this was landed, but this patch is way to complex for me to understand.

Mike Kaply might know more about the details how the Trackpoint driver works, since he said he talked to the developers about getting rid of some Mozilla hacks in  Bug 253115 .

Comment 135

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Bug 507222 seems to have been caused bug 506615 (backed out now).

Comment 136

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry, scratch that. Anyway, I'll deal with this in bug 507222.

Comment 137

[Karl Tomlinson (:karlt)]

Landed before 1.9.2a1 (based on bug 507222).