Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DevTools: Handle restricted browser pages properly like new tab page, extensions page etc(only chrome and edge for now) #20023

Merged
merged 6 commits into from
Oct 14, 2020
Merged

DevTools: Handle restricted browser pages properly like new tab page, extensions page etc(only chrome and edge for now) #20023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0635303
add support for detecting restricted browser page and show appropriat…
sktguha Oct 12, 2020
68b4c4e
import shared css in production.html also
sktguha Oct 12, 2020
510aafc
Merge branch 'master' of https://github.com/facebook/react into devto…
sktguha Oct 14, 2020
dfdf33d
run only for chrome and edge for now, not firefox and use better prot…
sktguha Oct 14, 2020
fbfa52e
do lint changes
sktguha Oct 14, 2020
c10241f
change some comments in the code as per review comment
sktguha Oct 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added packages/react-devtools-extensions/icons/128-restricted.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added packages/react-devtools-extensions/icons/16-restricted.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added packages/react-devtools-extensions/icons/32-restricted.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added packages/react-devtools-extensions/icons/48-restricted.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions packages/react-devtools-extensions/icons/restricted.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
6 changes: 1 addition & 5 deletions packages/react-devtools-extensions/popups/deadcode.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
<script src="shared.js"></script>
<link rel="stylesheet" href="shared.css" />
<style>
html, body {
font-size: 14px;
min-width: 460px;
min-height: 133px;
}

body {
margin: 8px;
}

hr {
width: 100%;
}
Expand Down
6 changes: 1 addition & 5 deletions packages/react-devtools-extensions/popups/development.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
<script src="shared.js"></script>
<link rel="stylesheet" href="shared.css" />
<style>
html, body {
font-size: 14px;
min-width: 460px;
min-height: 101px;
}

body {
margin: 8px;
}

hr {
width: 100%;
}
Expand Down
6 changes: 1 addition & 5 deletions packages/react-devtools-extensions/popups/disabled.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
<script src="shared.js"></script>
<link rel="stylesheet" href="shared.css" />
<style>
html, body {
font-size: 14px;
min-width: 410px;
min-height: 33px;
}

body {
margin: 8px;
}

hr {
width: 100%;
}
Expand Down
6 changes: 1 addition & 5 deletions packages/react-devtools-extensions/popups/outdated.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
<script src="shared.js"></script>
<link rel="stylesheet" href="shared.css" />
<style>
html, body {
font-size: 14px;
min-width: 460px;
min-height: 117px;
}

body {
margin: 8px;
}

hr {
width: 100%;
}
Expand Down
6 changes: 1 addition & 5 deletions packages/react-devtools-extensions/popups/production.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
<script src="shared.js"></script>
<link rel="stylesheet" href="shared.css" />
<style>
html, body {
font-size: 14px;
min-width: 460px;
min-height: 39px;
}

body {
margin: 8px;
}

hr {
width: 100%;
}
Expand Down
14 changes: 14 additions & 0 deletions packages/react-devtools-extensions/popups/restricted.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<script src="shared.js"></script>
<link rel="stylesheet" href="shared.css" />
<style>
html, body {
min-width: 286px;
min-height: 33px;
}

</style>
<p>
<b>This is a restricted browser page.</b>
<br />
React devtools cannot access this page.
</p>
7 changes: 7 additions & 0 deletions packages/react-devtools-extensions/popups/shared.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
html, body {
font-size: 14px;
}

body {
margin: 8px;
}
Copy link
Contributor

@bvaughn bvaughn Oct 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the shared CSS file if we aren't going to also share hr style?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so hr is not used everywhere in all pages. so basically I don't want to inject hr styles for nothing, if it is not used in the page. But if you feel so then I can do it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool :) , so we can keep it as it is I guess then. No changes needed here then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I guess it's not a big deal either way. DRY for CSS isn't really a big goal in this context though, so it just seemed weird to move some common styles but leave others.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I see. yes I mean I just saw it and thought would be good to extract into common file. Maybe if there are more usages of hr , we can think about extracting in future also.

42 changes: 34 additions & 8 deletions packages/react-devtools-extensions/src/background.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,17 +78,43 @@ function setIconAndPopup(reactBuildType, tabId) {
});
}

// Listen to URL changes on the active tab and reset the DevTools icon.
// This prevents non-disabled icons from sticking in Firefox.
// Don't listen to this event in Chrome though.
// It fires more frequently, often after onMessage() has been called.
if (IS_FIREFOX) {
chrome.tabs.onUpdated.addListener((tabId, changeInfo, tab) => {
function isRestrictedBrowserPage(url) {
return !url || new URL(url).protocol === 'chrome:';
}

function checkAndHandleRestrictedPageIfSo(tab) {
if (tab && isRestrictedBrowserPage(tab.url)) {
setIconAndPopup('restricted', tab.id);
}
}

// update popup page of any existing open tabs, if they are restricted browser pages.
// we can't update for any other types (prod,dev,outdated etc)
// as the content script needs to be injected at document_start itself for those kinds of detection
// TODO: Show a different popup page(to reload current page probably) for old tabs, opened before the extension is installed
if (!IS_FIREFOX) {
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
chrome.tabs.query({}, tabs => tabs.forEach(checkAndHandleRestrictedPageIfSo));
chrome.tabs.onCreated.addListener((tabId, changeInfo, tab) =>
checkAndHandleRestrictedPageIfSo(tab),
);
}

// Listen to URL changes on the active tab and update the DevTools icon.
chrome.tabs.onUpdated.addListener((tabId, changeInfo, tab) => {
if (IS_FIREFOX) {
// We don't properly detect protected URLs in Firefox at the moment.
// However we can reset the DevTools icon to its loading state when the URL changes.
// It will be updated to the correct icon by the onMessage callback below.
if (tab.active && changeInfo.status === 'loading') {
setIconAndPopup('disabled', tabId);
}
});
}
} else {
// Don't reset the icon to the loading state for Chrome or Edge.
// The onUpdated callback fires more frequently for these browsers,
// often after onMessage has been called.
checkAndHandleRestrictedPageIfSo(tab);
}
});

chrome.runtime.onMessage.addListener((request, sender) => {
if (sender.tab) {
Expand Down