Skip to content

Commit

Permalink
Revert "Add viewport scroll information to PDFAccessibilityTree"
Browse files Browse the repository at this point in the history
This reverts commit c2f0727.

Reason for revert: This change is causing regression. Bounding box
calculation of a given element requires the |kScrollX| and |kScrollY|
positions. In case of PDF there is a scroll element which lives outside
the PDF node. The scroll element also reports scroll positions. During
calculation of bounding boxes in AXTree::RelativeToTreeBoundsInternal
|kScrollX| and |kScrollY| are applied twice.

Original change's description:
> Add viewport scroll information to PDFAccessibilityTree
>
> This CL adds following information to PDFAccessibilityTree
> - ax::mojom::IntAttribute::kScrollXMin
> - ax::mojom::IntAttribute::kScrollXMax
> - ax::mojom::IntAttribute::kScrollX
> - ax::mojom::IntAttribute::kScrollYMin
> - ax::mojom::IntAttribute::kScrollYMax
> - ax::mojom::IntAttribute::kScrollY
>
> It enables partial implementation of IScrollProvider, following methods
> will work with this change
> - IScrollProvider::get_HorizontallyScrollable
> - IScrollProvider::get_VerticallyScrollable
> - IScrollProvider::get_HorizontalScrollPercent
> - IScrollProvider::get_VerticalScrollPercent
>
> This information is required by Screen readers to announce the current
> scroll position and extent of scroll to the user.
>
> Tests: The best way to test this change will be through browser tests.
> However, it requires the second part of the change, detailed in the
> associated bug, to be completed.
>
> Bug: 1034521
> Change-Id: I629b2773c75f2b6b222d339cd0ae01799d695295
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1985821
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Lei Zhang <thestig@chromium.org>
> Commit-Queue: Virender Singh <virens@microsoft.com>
> Cr-Commit-Position: refs/heads/master@{#728843}

TBR=dcheng@chromium.org,thestig@chromium.org,mohitb@microsoft.com,virens@microsoft.com

# Not skipping CQ checks because original CL landed > 1 day ago.

(cherry picked from commit 05d6d36)

Bug: 1034521
Change-Id: I15675f9ba604be96e9c50c9bea1dbe84ef8c9a0a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2041714
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#739108}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2043339
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/branch-heads/4044@{#198}
Cr-Branched-From: a6d9daf-refs/heads/master@{#737173}
  • Loading branch information
virenms authored and Commit Bot committed Feb 11, 2020
1 parent 9267226 commit 2ceb750
Show file tree
Hide file tree
Showing 11 changed files with 7 additions and 56 deletions.
10 changes: 0 additions & 10 deletions components/pdf/renderer/pdf_accessibility_tree.cc
Expand Up @@ -362,16 +362,6 @@ void PdfAccessibilityTree::SetAccessibilityViewportInfo(
if (render_accessibility && tree_.size() > 1) {
ui::AXNode* root = tree_.root();
ui::AXNodeData root_data = root->data();
root_data.AddIntAttribute(ax::mojom::IntAttribute::kScrollXMin, 0);
root_data.AddIntAttribute(ax::mojom::IntAttribute::kScrollXMax,
viewport_info.total_scrollable_size.width);
root_data.AddIntAttribute(ax::mojom::IntAttribute::kScrollX,
viewport_info.current_scroll_position.x);
root_data.AddIntAttribute(ax::mojom::IntAttribute::kScrollYMin, 0);
root_data.AddIntAttribute(ax::mojom::IntAttribute::kScrollYMax,
viewport_info.total_scrollable_size.height);
root_data.AddIntAttribute(ax::mojom::IntAttribute::kScrollY,
viewport_info.current_scroll_position.y);
root_data.relative_bounds.transform =
base::WrapUnique(MakeTransformFromViewInfo());
root->SetData(root_data);
Expand Down
2 changes: 0 additions & 2 deletions components/pdf/renderer/pdf_accessibility_tree_browsertest.cc
Expand Up @@ -173,8 +173,6 @@ class PdfAccessibilityTreeTest : public content::RenderViewTest {

viewport_info_.zoom = 1.0;
viewport_info_.scale = 1.0;
viewport_info_.total_scrollable_size = {0, 0};
viewport_info_.current_scroll_position = {0, 0};
viewport_info_.scroll = {0, 0};
viewport_info_.offset = {0, 0};
viewport_info_.selection_start_page_index = 0;
Expand Down
25 changes: 1 addition & 24 deletions pdf/out_of_process_instance.cc
Expand Up @@ -1017,10 +1017,6 @@ void OutOfProcessInstance::SendAccessibilityViewportInfo() {

viewport_info.zoom = zoom_;
viewport_info.scale = device_scale_;
viewport_info.total_scrollable_size = {GetTotalScrollableWidth(),
GetTotalScrollableHeight()};
viewport_info.current_scroll_position = {GetHorizontalScrollPosition(),
GetVerticalScrollPosition()};

engine_->GetSelection(&viewport_info.selection_start_page_index,
&viewport_info.selection_start_char_index,
Expand Down Expand Up @@ -1303,25 +1299,6 @@ int OutOfProcessInstance::GetDocumentPixelHeight() const {
ceil(document_size_.height() * zoom_ * device_scale_));
}

int OutOfProcessInstance::GetTotalScrollableWidth() const {
return std::max(GetDocumentPixelWidth() - plugin_size_.width(), 0);
}

int OutOfProcessInstance::GetTotalScrollableHeight() const {
return std::max(static_cast<int>(GetDocumentPixelHeight() +
GetToolbarHeightInScreenCoords() -
plugin_size_.height()),
0);
}

int OutOfProcessInstance::GetHorizontalScrollPosition() const {
return static_cast<int>(scroll_offset_.x() * device_scale_);
}

int OutOfProcessInstance::GetVerticalScrollPosition() const {
return static_cast<int>(scroll_offset_.y() * device_scale_);
}

void OutOfProcessInstance::FillRect(const pp::Rect& rect, uint32_t color) {
DCHECK(!image_data_.is_null() || rect.IsEmpty());
uint32_t* buffer_start = static_cast<uint32_t*>(image_data_.data());
Expand Down Expand Up @@ -2017,7 +1994,7 @@ void OutOfProcessInstance::IsEditModeChanged(bool is_edit_mode) {
pp::PDF::SetPluginCanSave(this, ShouldSaveEdits());
}

float OutOfProcessInstance::GetToolbarHeightInScreenCoords() const {
float OutOfProcessInstance::GetToolbarHeightInScreenCoords() {
return top_toolbar_height_in_viewport_coords_ * device_scale_;
}

Expand Down
10 changes: 1 addition & 9 deletions pdf/out_of_process_instance.h
Expand Up @@ -149,7 +149,7 @@ class OutOfProcessInstance : public pp::Instance,
void IsSelectingChanged(bool is_selecting) override;
void SelectionChanged(const pp::Rect& left, const pp::Rect& right) override;
void IsEditModeChanged(bool is_edit_mode) override;
float GetToolbarHeightInScreenCoords() const override;
float GetToolbarHeightInScreenCoords() override;

// PreviewModeClient::Client implementation.
void PreviewDocumentLoadComplete() override;
Expand Down Expand Up @@ -180,14 +180,6 @@ class OutOfProcessInstance : public pp::Instance,
int GetDocumentPixelWidth() const;
int GetDocumentPixelHeight() const;

// Computes total scrollable Width and Height of the document.
int GetTotalScrollableWidth() const;
int GetTotalScrollableHeight() const;

// Computes current horizontal and scroll position of the document.
int GetHorizontalScrollPosition() const;
int GetVerticalScrollPosition() const;

// Draws a rectangle with the specified dimensions and color in our buffer.
void FillRect(const pp::Rect& rect, uint32_t color);

Expand Down
2 changes: 1 addition & 1 deletion pdf/pdf_engine.h
Expand Up @@ -282,7 +282,7 @@ class PDFEngine {

// Gets the height of the top toolbar in screen coordinates. This is
// independent of whether it is hidden or not at the moment.
virtual float GetToolbarHeightInScreenCoords() const = 0;
virtual float GetToolbarHeightInScreenCoords() = 0;
};

struct AccessibilityLinkInfo {
Expand Down
2 changes: 1 addition & 1 deletion pdf/preview_mode_client.cc
Expand Up @@ -150,7 +150,7 @@ bool PreviewModeClient::IsPrintPreview() {
return false;
}

float PreviewModeClient::GetToolbarHeightInScreenCoords() const {
float PreviewModeClient::GetToolbarHeightInScreenCoords() {
return 0.0f;
}

Expand Down
2 changes: 1 addition & 1 deletion pdf/preview_mode_client.h
Expand Up @@ -67,7 +67,7 @@ class PreviewModeClient : public PDFEngine::Client {
void DocumentHasUnsupportedFeature(const std::string& feature) override;
void FormTextFieldFocusChange(bool in_focus) override;
bool IsPrintPreview() override;
float GetToolbarHeightInScreenCoords() const override;
float GetToolbarHeightInScreenCoords() override;
uint32_t GetBackgroundColor() override;

private:
Expand Down
2 changes: 1 addition & 1 deletion pdf/test/test_client.cc
Expand Up @@ -56,7 +56,7 @@ uint32_t TestClient::GetBackgroundColor() {
return 0;
}

float TestClient::GetToolbarHeightInScreenCoords() const {
float TestClient::GetToolbarHeightInScreenCoords() {
return 0;
}

Expand Down
2 changes: 1 addition & 1 deletion pdf/test/test_client.h
Expand Up @@ -37,7 +37,7 @@ class TestClient : public PDFEngine::Client {
pp::Instance* GetPluginInstance() override;
bool IsPrintPreview() override;
uint32_t GetBackgroundColor() override;
float GetToolbarHeightInScreenCoords() const override;
float GetToolbarHeightInScreenCoords() override;

private:
// Not owned. Expected to dangle briefly, as the engine usually is destroyed
Expand Down
4 changes: 0 additions & 4 deletions ppapi/c/private/ppb_pdf.h
Expand Up @@ -36,10 +36,6 @@ struct PP_PrivateFindResult {
struct PP_PrivateAccessibilityViewportInfo {
double zoom;
double scale;
// |total_scrollable_size| and |current_scroll_position| are relative
// to plugin embed and in screen coordinates.
struct PP_Size total_scrollable_size;
struct PP_Point current_scroll_position;
struct PP_Point scroll;
struct PP_Point offset;
uint32_t selection_start_page_index;
Expand Down
2 changes: 0 additions & 2 deletions ppapi/proxy/ppapi_messages.h
Expand Up @@ -268,8 +268,6 @@ IPC_STRUCT_TRAITS_END()
IPC_STRUCT_TRAITS_BEGIN(PP_PrivateAccessibilityViewportInfo)
IPC_STRUCT_TRAITS_MEMBER(zoom)
IPC_STRUCT_TRAITS_MEMBER(scale)
IPC_STRUCT_TRAITS_MEMBER(total_scrollable_size)
IPC_STRUCT_TRAITS_MEMBER(current_scroll_position)
IPC_STRUCT_TRAITS_MEMBER(scroll)
IPC_STRUCT_TRAITS_MEMBER(offset)
IPC_STRUCT_TRAITS_MEMBER(selection_start_page_index)
Expand Down

0 comments on commit 2ceb750

Please sign in to comment.