Fix overlapping annot rendering with focused annot. The reason CPDFSDK_AnnotReverseIteration existed was to put the focused annotation at the end of the list, so it will draw on top of other annotations. When https://pdfium-review.googlesource.com/98870 fixed the drawing order for overlapping annotations, it broke this use case. Add CPDFSDK_AnnotIteration::CreateForDrawing() as a factory function to create CPDFSDK_AnnotIteration objects that iterate correctly for the drawing use case. Use it in CPDFSDK_PageView::PageView_OnDraw() to fix annotation drawing. Make sure it works as expected with a new test case. Bug: chromium:1372651 Change-Id: I0bf673e9af9eb59773f7ef933fc363aab9f22abc Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/99110 Commit-Queue: Lei Zhang <thestig@chromium.org> Reviewed-by: Tom Sepez <tsepez@chromium.org>
diff --git a/fpdfsdk/cpdfsdk_annotiteration.cpp b/fpdfsdk/cpdfsdk_annotiteration.cpp index aa736bb..e32946f 100644 --- a/fpdfsdk/cpdfsdk_annotiteration.cpp +++ b/fpdfsdk/cpdfsdk_annotiteration.cpp
@@ -11,26 +11,39 @@ #include "fpdfsdk/cpdfsdk_annot.h" #include "fpdfsdk/cpdfsdk_pageview.h" -CPDFSDK_AnnotIteration::CPDFSDK_AnnotIteration(CPDFSDK_PageView* pPageView) { +// static +CPDFSDK_AnnotIteration CPDFSDK_AnnotIteration::CreateForDrawing( + CPDFSDK_PageView* page_view) { + CPDFSDK_AnnotIteration result(page_view); + return CPDFSDK_AnnotIteration(page_view, /*put_focused_annot_at_end=*/true); +} + +CPDFSDK_AnnotIteration::CPDFSDK_AnnotIteration(CPDFSDK_PageView* page_view) + : CPDFSDK_AnnotIteration(page_view, /*put_focused_annot_at_end=*/false) {} + +CPDFSDK_AnnotIteration::CPDFSDK_AnnotIteration(CPDFSDK_PageView* page_view, + bool put_focused_annot_at_end) { // Copying ObservedPtrs is expensive, so do it once at the end. - std::vector<CPDFSDK_Annot*> copied_list = pPageView->GetAnnotList(); + std::vector<CPDFSDK_Annot*> copied_list = page_view->GetAnnotList(); std::stable_sort(copied_list.begin(), copied_list.end(), [](const CPDFSDK_Annot* p1, const CPDFSDK_Annot* p2) { return p1->GetLayoutOrder() < p2->GetLayoutOrder(); }); - CPDFSDK_Annot* pTopMostAnnot = pPageView->GetFocusAnnot(); + CPDFSDK_Annot* pTopMostAnnot = page_view->GetFocusAnnot(); if (pTopMostAnnot) { auto it = std::find(copied_list.begin(), copied_list.end(), pTopMostAnnot); if (it != copied_list.end()) { copied_list.erase(it); - copied_list.insert(copied_list.begin(), pTopMostAnnot); + auto insert_it = + put_focused_annot_at_end ? copied_list.end() : copied_list.begin(); + copied_list.insert(insert_it, pTopMostAnnot); } } - m_List.reserve(copied_list.size()); + list_.reserve(copied_list.size()); for (auto* pAnnot : copied_list) - m_List.emplace_back(pAnnot); + list_.emplace_back(pAnnot); } CPDFSDK_AnnotIteration::~CPDFSDK_AnnotIteration() = default;
diff --git a/fpdfsdk/cpdfsdk_annotiteration.h b/fpdfsdk/cpdfsdk_annotiteration.h index a222877..ff5b99a 100644 --- a/fpdfsdk/cpdfsdk_annotiteration.h +++ b/fpdfsdk/cpdfsdk_annotiteration.h
@@ -18,14 +18,21 @@ using const_iterator = std::vector<ObservedPtr<CPDFSDK_Annot>>::const_iterator; - explicit CPDFSDK_AnnotIteration(CPDFSDK_PageView* pPageView); + static CPDFSDK_AnnotIteration CreateForDrawing(CPDFSDK_PageView* page_view); + + explicit CPDFSDK_AnnotIteration(CPDFSDK_PageView* page_view); + CPDFSDK_AnnotIteration(const CPDFSDK_AnnotIteration&) = delete; + CPDFSDK_AnnotIteration& operator=(const CPDFSDK_AnnotIteration&) = delete; ~CPDFSDK_AnnotIteration(); - const_iterator begin() const { return m_List.begin(); } - const_iterator end() const { return m_List.end(); } + const_iterator begin() const { return list_.begin(); } + const_iterator end() const { return list_.end(); } private: - std::vector<ObservedPtr<CPDFSDK_Annot>> m_List; + CPDFSDK_AnnotIteration(CPDFSDK_PageView* page_view, + bool put_focused_annot_at_end); + + std::vector<ObservedPtr<CPDFSDK_Annot>> list_; }; #endif // FPDFSDK_CPDFSDK_ANNOTITERATION_H_
diff --git a/fpdfsdk/cpdfsdk_pageview.cpp b/fpdfsdk/cpdfsdk_pageview.cpp index 66ed4cf..2f35101 100644 --- a/fpdfsdk/cpdfsdk_pageview.cpp +++ b/fpdfsdk/cpdfsdk_pageview.cpp
@@ -86,7 +86,7 @@ #endif // PDF_ENABLE_XFA // for pdf/static xfa. - CPDFSDK_AnnotIteration annot_iteration(this); + auto annot_iteration = CPDFSDK_AnnotIteration::CreateForDrawing(this); for (const auto& pSDKAnnot : annot_iteration) { pSDKAnnot->OnDraw(pDevice, mtUser2Device, pOptions->GetDrawAnnots()); }
diff --git a/testing/resources/pixel/bug_1372651.evt b/testing/resources/pixel/bug_1372651.evt new file mode 100644 index 0000000..eb762eb --- /dev/null +++ b/testing/resources/pixel/bug_1372651.evt
@@ -0,0 +1,4 @@ +# Open the dropdown +mousemove,140,145 +mousedown,left,140,145 +mouseup,left,140,145
diff --git a/testing/resources/pixel/bug_1372651.in b/testing/resources/pixel/bug_1372651.in new file mode 100644 index 0000000..95b1948 --- /dev/null +++ b/testing/resources/pixel/bug_1372651.in
@@ -0,0 +1,71 @@ +{{header}} +{{object 1 0}} << + /Type /Catalog + /AcroForm << + /DA (/Helv 0 Tf 0 g) + /DR << + /Font << + /Helv 4 0 R + >> + >> + /Fields [5 0 R 6 0 R] + >> + /Pages 2 0 R +>> +endobj +{{object 2 0}} << + /Type /Pages + /Count 1 + /Kids [3 0 R] +>> +endobj +{{object 3 0}} << + /Type /Page + /Parent 2 0 R + /Annots [5 0 R 6 0 R] + /MediaBox [0 0 200 200] +>> +endobj +{{object 4 0}} << + /Type /Font + /Subtype /Type1 + /BaseFont /Helvetica +>> +endobj +{{object 5 0}} << + /Type /Annot + /Subtype /Widget + /FT /Ch + /DV (Item3) + /F 4 + /Ff 131072 + /I [2] + /MK << + /BG [1.0] + >> + /Opt [(Item1) (Item2) (Item3)] + /P 3 0 R + /Rect [70 135 150 155] + /T (Dropdown) + /V (Item3) +>> +endobj +{{object 6 0}} << + /Type /Annot + /Subtype /Widget + /FT /Btn + /F 4 + /Ff 65536 + /MK << + /BG [0.5] + >> + /P 3 0 R + /Rect [50 90 150 110] + /T (Button) + /TU (Button Tooltip) +>> +endobj +{{xref}} +{{trailer}} +{{startxref}} +%%EOF
diff --git a/testing/resources/pixel/bug_1372651_expected.pdf.0.png b/testing/resources/pixel/bug_1372651_expected.pdf.0.png new file mode 100644 index 0000000..c7386a1 --- /dev/null +++ b/testing/resources/pixel/bug_1372651_expected.pdf.0.png Binary files differ
diff --git a/testing/resources/pixel/bug_1372651_expected_mac.pdf.0.png b/testing/resources/pixel/bug_1372651_expected_mac.pdf.0.png new file mode 100644 index 0000000..a749d63 --- /dev/null +++ b/testing/resources/pixel/bug_1372651_expected_mac.pdf.0.png Binary files differ
diff --git a/testing/resources/pixel/bug_1372651_expected_skia.pdf.0.png b/testing/resources/pixel/bug_1372651_expected_skia.pdf.0.png new file mode 100644 index 0000000..00cc13c --- /dev/null +++ b/testing/resources/pixel/bug_1372651_expected_skia.pdf.0.png Binary files differ
diff --git a/testing/resources/pixel/bug_1372651_expected_skiapaths.pdf.0.png b/testing/resources/pixel/bug_1372651_expected_skiapaths.pdf.0.png new file mode 100644 index 0000000..f33a4a7 --- /dev/null +++ b/testing/resources/pixel/bug_1372651_expected_skiapaths.pdf.0.png Binary files differ
