Gecko VBufBackend: greatly speed up subtree re-renders when part of a document has changed

nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.cpp

@@ -120,7 +120,7 @@ template<typename TableType> inline void fillTableCounts(VBufStorage_controlFiel
 	}
 }
 
-inline int updateTableCounts(IAccessibleTableCell* tableCell, VBufStorage_buffer_t* tableBuffer) {
+inline int getTableIDFromCell(IAccessibleTableCell* tableCell) {
 	IUnknown* unk = NULL;
 	if (tableCell->get_table(&unk) != S_OK || !unk)
 		return 0;
@@ -130,26 +130,8 @@ inline int updateTableCounts(IAccessibleTableCell* tableCell, VBufStorage_buffer
 	unk->Release();
 	if (res != S_OK || !acc)
 		return 0;
-	HWND docHwnd;
-	int id;
-	if (acc->get_windowHandle(&docHwnd) != S_OK
-			|| acc->get_uniqueID((long*)&id) != S_OK) {
-		acc->Release();
-		return 0;
-	}
-	const int docHandle = HandleToUlong(docHwnd);
-	VBufStorage_controlFieldNode_t* node = tableBuffer->getControlFieldNodeWithIdentifier(docHandle, id);
-	if (!node) {
-		acc->Release();
-		return 0;
-	}
-	IAccessibleTable2* table = NULL;
-	if (acc->QueryInterface(IID_IAccessibleTable2, (void**)&table) != S_OK || !table) {
-		acc->Release();
-		return 0;
-	}
-	fillTableCounts<IAccessibleTable2>(node, acc, table);
-	table->Release();
+	int id=0;
+	acc->get_uniqueID((long*)&id);
 	acc->Release();
 	return id;
 }
@@ -394,15 +376,6 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf(IAccessible2* pacc,
 	nhAssert(parentNode); //new node must have been created
 	previousNode=NULL;
 
-	if(paccTable2) {
-		LOG_DEBUG(L"Setting node's denyReuseIfPreviousSiblingsChanged to true");
-		parentNode->denyReuseIfPreviousSiblingsChanged=true;
-		LOG_DEBUG(L"Setting node's requiresParentUpdate to true");
-		parentNode->requiresParentUpdate=true;
-		LOG_DEBUG(L"Setting node's alwaysRerenderChildren to true");
-		parentNode->alwaysRerenderChildren=true;
-	}
-
 	//Get role -- IAccessible2 role
 	long role=0;
 	BSTR roleString=NULL;
@@ -681,11 +654,28 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf(IAccessible2* pacc,
 		paccTableCell=nullptr;
 	}
 
-	if(paccTableCell) {
-		LOG_DEBUG(L"Setting node's requiresParentUpdate back to false as this is a table cell");
-		parentNode->requiresParentUpdate=false;
-		LOG_DEBUG(L"Setting node's alwaysRerenderChildren back to false as this is a table cell");
-		parentNode->alwaysRerenderChildren=false;
+	if(paccTable2) {
+		// We are rendering a node that is part of a table (row group, row or cell).
+		// Set some properties to ensure that this and other nodes in the table aare correctly re-rendered if the table changes,
+		// so that the table's row and column cordinates remain accurate.
+		// setting denyReuseIfPreviousSiblingsChange ensures that if any part of the table is added or removed previous to this node,
+		// his node will not be reused (as its row / column coordinates would now be out of date).
+		LOG_DEBUG(L"Setting node's denyReuseIfPreviousSiblingsChanged to true");
+		parentNode->denyReuseIfPreviousSiblingsChanged=true;
+		if(!paccTableCell) { // just rows and row groups
+			// setting requiresParentUpdate ensures that if this node is specifically invalidated,
+			// its parent will also be invalidated.
+			// For example, if this is a table row, its rerendering may change the number of cells inside. 
+			// this in tern would affect the coordinates of all table cells in table rows after this row.
+			// Thus, ensuring we rerender this node's parent, gives a chance to rerender other table rows.
+			LOG_DEBUG(L"Setting node's requiresParentUpdate to true");
+			parentNode->requiresParentUpdate=true;
+			// Setting alwaysRerenderChildren ensures that if this node is rerendered, none of its children are reused.
+			// For example, if this is a table row that is rerendered (perhaps due to a previous table row being added),
+			// this row's cells can't be reused because their coordinates would now be out of date.
+			LOG_DEBUG(L"Setting node's alwaysRerenderChildren to true");
+			parentNode->alwaysRerenderChildren=true;
+		}
 	}
 
 	// For IAccessibleTable, we must always be passed the table interface by the caller.
@@ -700,7 +690,7 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf(IAccessible2* pacc,
 			this->fillTableCellInfo_IATable2(parentNode, paccTableCell);
 			if (!paccTable2) {
 				// This is an update; we're not rendering the entire table.
-				tableID = updateTableCounts(paccTableCell, this);
+				tableID = getTableIDFromCell(paccTableCell);
 			}
 			paccTableCell->Release();
 			paccTableCell = NULL;

nvdaHelper/vbufBase/backend.cpp

@@ -186,10 +186,6 @@ bool VBufBackend_t::invalidateSubtree(VBufStorage_controlFieldNode_t* node) {
 }
 
 void VBufBackend_t::update() {
-	if(inUpdate) {
-		return;
-	}
-	inUpdate=true;
 	if(this->hasContent()) {
 		this->lock.acquire();
 		LOG_DEBUG(L"Updating "<<pendingInvalidSubtreesList.size()<<L" subtrees");
@@ -227,7 +223,6 @@ void VBufBackend_t::update() {
 		this->lock.release();
 	}
 	LOG_DEBUG(L"Update complete");
-	inUpdate=false;
 }
 
 void VBufBackend_t::terminate() {
@@ -278,13 +273,19 @@ VBufStorage_controlFieldNode_t* VBufBackend_t::reuseExistingNodeInRender(VBufSto
 		return nullptr;
 	}
 	if(existingNode->denyReuseIfPreviousSiblingsChanged) {
+		// This node is not allowed to be reused if any of its previous siblings have changed.
+		// We work this out by walking back to the previous controlFieldNode in its siblings, and ensuring that it is a reference node that references the existing node's first previous controlFieldNode.
+		// As we know that buffers are always rendered in a forward direction, we can garantee that if the previous controlFieldNode is correct,
+		// then all previous nodes before that are also correct.
 		VBufStorage_controlFieldNode_t* previousControlFieldNode=nullptr;
 		for(auto tempNode=previous;tempNode!=nullptr;tempNode=tempNode->getPrevious()) {
 			previousControlFieldNode=dynamic_cast<VBufStorage_controlFieldNode_t*>(tempNode);
 			if(previousControlFieldNode) break;
 		}
 		VBufStorage_referenceNode_t* previousReferenceNode=dynamic_cast<VBufStorage_referenceNode_t*>(previousControlFieldNode);
 		if(previousControlFieldNode&&!previousReferenceNode) {
+			// This is a controlFieldNode but not a referenceNode.
+			// Therefore this node has been newly added.
 			LOG_DEBUG(L"Previous controlFieldNode was not a referenceNode");
 			return nullptr;
 		}
@@ -297,6 +298,9 @@ VBufStorage_controlFieldNode_t* VBufBackend_t::reuseExistingNodeInRender(VBufSto
 			if(previousExistingControlFieldNode) break;
 		}
 		if(previousControlFieldNode!=previousExistingControlFieldNode) {
+			// The previous node differs from the existing previous node.
+			// We already know its not because a node was added, therefore this must be either a removal or a move.
+			// either way, this means that  the given node's previous siblings have changed.
 			LOG_DEBUG(L"Previous controlFieldNodes differ");
 			return nullptr;
 		}

nvdaHelper/vbufBase/backend.h

@@ -104,11 +104,6 @@ static LRESULT CALLBACK destroy_callWndProcHook(int code, WPARAM wParam, LPARAM
  */
 	virtual void render(VBufStorage_buffer_t* buffer, int docHandle, int ID, VBufStorage_controlFieldNode_t* oldNode=NULL)=0;
 
-/**
- * Tracks if the  backend is currently being updated, and if so does not allow update to become reentrant.
-  */
-	bool inUpdate {false};
-
 /**
  * Updates the content of the buffer. 
  * If no content yet exists it renders the entire document. If content exists it only re-renders nodes marked as invalid.

nvdaHelper/vbufBase/storage.h

@@ -357,7 +357,7 @@ class VBufStorage_controlFieldNode_t : public VBufStorage_fieldNode_t {
  */
 	bool alwaysRerenderChildren {false};
 
-/*
+/**
  * If true, all this node's descendants will always be re-rendered along with this node when being re-rendered.
  */
 	bool alwaysRerenderDescendants {false};

source/NVDAObjects/IAccessible/mozilla.py

@@ -34,7 +34,7 @@ def _getPhysicalFocus(self):
 	def setFocus(self):
 		oldFocus=self._getPhysicalFocus()
 		super(Mozilla,self).setFocus()
-		# Although all versions of Firefox block in setFocus until the physical focus has moved,
+		# Although all versions of Firefox block inSetFocus or in accFocus until    the physical focus has moved,
 		# Firefox 57 and above return before they fire a focus winEvent communicating the focus change to ATs.
 		# Therefore, If the call to setFocus did change the physical focus,
 		# Wait for a focus event to be queued to NVDA before returning.