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
