Skip to content

Commit

Permalink
[Unified PDF] Form control partial repaint for "related" fields is br…
Browse files Browse the repository at this point in the history
…oken, results in fields getting stuck.

https://bugs.webkit.org/show_bug.cgi?id=270539
rdar://123637341

Reviewed by Abrar Rahman Protyasha and Tim Horton.

In order to avoid repainting the world we attempt to perform very
targeted repaints. For annotations this generally means we need to find
the bounds for the annotation and provide the geometry of the area to
the async renderer to that it can repaint the affected area. However,
certain annotations may also have an effect on a set of related
annotations. For example, when interacting with radio buttons in the
same group we want to make sure that not only is the newly selected
radio button displayed as active but also that the previously selected one
is no longer active. For these annotations we want to make sure that
those get repainted as well in order to properly display the state
changes.

Since we already have an observer that listens for form field changes
that are sent by the document, we can use the same notification to tell
the plugin of the field that changed and get the geometry of the
annotations. We can take the field name given by the observer and ask
the document for all of the annotations associated with this field name
through IPI.

In order for the repainting to happen correctly we had to slightly
change how the async renderer handled requests for tile repaints.
Previously, it would check to see if a repaint request for a particular
tile had already been enqueued and drop the current request if it had.
This approach was too conservative since multiple annotations may need
to get repainted within a tile and observer gets notified of multiple
fields independently (e.g. a button action that resets form fields
sends a separate notification for each field in the form). Now the
async observer will drop the request if:

1.) There is not a clip rect provided for the request (to match current
behavior)
2.) The tile rect clip for the repaint request is already contained
completely within the tile rect clip for the enqueued tile update

If a repaint request comes in for the same tile but it is not being
handled by an enqueued tile update then we will take the existing
update's tile rect, combine it with the rect in the new request, and
update the enqueued tile update. As we complete the tile update we will
check to see if the area that was repainted is the same as the one that
was enqueued. If it is not this means that the tile update was updated
with a new rect so we do not proceed to cache the tile.

* Source/WebKit/Platform/spi/Cocoa/PDFKitSPI.h:
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/AsyncPDFRenderer.mm:
(WebKit::AsyncPDFRenderer::didCompleteTileUpdateRender):
(WebKit::AsyncPDFRenderer::updateTilesForPaintingRect):
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/UnifiedPDFPlugin.h:
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/UnifiedPDFPlugin.mm:
(-[WKPDFFormMutationObserver formChanged:]):
(WebKit::UnifiedPDFPlugin::repaintAnnotationsForFormField):

Canonical link: https://commits.webkit.org/275727@main
  • Loading branch information
sammygill committed Mar 6, 2024
1 parent 0f76043 commit d583f18
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 8 deletions.
1 change: 1 addition & 0 deletions Source/WebKit/Platform/spi/Cocoa/PDFKitSPI.h
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@
#if ENABLE(UNIFIED_PDF)
@interface PDFDocument (IPI)
- (PDFDestination *)namedDestination:(NSString *)name;
- (NSArray *)annotationsForFieldName:(NSString *)fieldname;
@end

#if HAVE(COREGRAPHICS_WITH_PDF_AREA_OF_INTEREST_SUPPORT)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -425,15 +425,16 @@
<< " (" << m_rendereredTiles.size() << " tiles in cache). Request removed " << !m_enqueuedTileUpdates.contains(tileInfo)
<< " configuration changed " << (renderInfo.configurationIdentifier != m_currentConfigurationIdentifier));

auto enqueuedTileUpdatesItr = m_enqueuedTileUpdates.find(tileInfo);

// If the request for this tile has been revoked, don't cache it.
if (!m_enqueuedTileUpdates.contains(tileInfo))
if (enqueuedTileUpdatesItr == m_enqueuedTileUpdates.end() || enqueuedTileUpdatesItr->value.clipRect != renderInfo.clipRect)
return;

// Configuration has changed.
if (renderInfo.configurationIdentifier != m_currentConfigurationIdentifier) {
auto it = m_enqueuedTileUpdates.find(tileInfo);
if (it != m_enqueuedTileUpdates.end() && it->value.configurationIdentifier == renderInfo.configurationIdentifier)
m_enqueuedTileUpdates.remove(it);
if (enqueuedTileUpdatesItr->value.configurationIdentifier == renderInfo.configurationIdentifier)
m_enqueuedTileUpdates.remove(enqueuedTileUpdatesItr);
return;
}

Expand Down Expand Up @@ -560,12 +561,17 @@
continue;

auto it = m_enqueuedTileUpdates.find(tileInfo);
if (it != m_enqueuedTileUpdates.end() && it->value.configurationIdentifier == m_currentConfigurationIdentifier)
return;

auto tileClipRect = intersection(renderedTile.tileInfo.tileRect, paintingRectInTileCoordinates);

if (it != m_enqueuedTileUpdates.end() && it->value.configurationIdentifier) {
if (!it->value.clipRect || it->value.clipRect->contains(tileClipRect))
return;

tileClipRect.unite(it->value.clipRect.value());
}

auto tileRenderInfo = TileRenderInfo { renderedTile.tileInfo.tileRect, tileClipRect, pageCoverage, m_currentConfigurationIdentifier };
m_enqueuedTileUpdates.add(tileInfo, tileRenderInfo);
m_enqueuedTileUpdates.set(tileInfo, tileRenderInfo);

LOG_WITH_STREAM(PDFAsyncRendering, stream << "AsyncPDFRenderer::updateTilesForPaintingRect " << paintingRect << " - enqueue update for tile " << tileInfo
<< " tile clip " << tileClipRect << " in " << renderedTile.tileInfo.tileRect);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ class UnifiedPDFPlugin final : public PDFPluginBase, public WebCore::GraphicsLay
#endif
enum class IsAnnotationCommit : bool { No, Yes };
static OptionSet<RepaintRequirement> repaintRequirementsForAnnotation(PDFAnnotation *, IsAnnotationCommit = IsAnnotationCommit::No);
void repaintAnnotationsForFormField(NSString *fieldName);

void attemptToUnlockPDF(const String& password) final;
void windowActivityDidChange() final;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,9 @@ - (id)initWithPlugin:(WebKit::UnifiedPDFPlugin *)plugin
- (void)formChanged:(NSNotification *)notification
{
_plugin->didMutatePDFDocument();

NSString *fieldName = (NSString *)[[notification userInfo] objectForKey:@"PDFFormFieldName"];
_plugin->repaintAnnotationsForFormField(fieldName);
}
@end

Expand Down Expand Up @@ -1933,6 +1936,13 @@ static bool isContextMenuEvent(const WebMouseEvent& event)
return { };
}

void UnifiedPDFPlugin::repaintAnnotationsForFormField(NSString *fieldName)
{
RetainPtr annotations = [m_pdfDocument annotationsForFieldName:fieldName];
for (PDFAnnotation *annotation in annotations.get())
setNeedsRepaintInDocumentRect(repaintRequirementsForAnnotation(annotation), documentRectForAnnotation(annotation));
}

WebCore::FloatRect UnifiedPDFPlugin::documentRectForAnnotation(PDFAnnotation *annotation) const
{
if (!annotation)
Expand Down

0 comments on commit d583f18

Please sign in to comment.