-
Notifications
You must be signed in to change notification settings - Fork 46.5k
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.
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
DevTools: Handle restricted browser pages properly like new tab page, extensions page etc(only chrome and edge for now) #20023
Changes from 5 commits
0635303
68b4c4e
510aafc
dfdf33d
fbfa52e
c10241f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
html, body { | ||
font-size: 14px; | ||
} | ||
|
||
body { | ||
margin: 8px; | ||
} | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,17 +78,40 @@ 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), | ||
); | ||
} | ||
|
||
chrome.tabs.onUpdated.addListener((tabId, changeInfo, tab) => { | ||
// Listen to URL changes on the active tab and reset the DevTools icon. | ||
// This prevents non-disabled icons from sticking in Firefox. | ||
// Don't do this in Chrome or Edge though. | ||
// It fires more frequently, often after onMessage() has been called. | ||
if (IS_FIREFOX) { | ||
if (tab.active && changeInfo.status === 'loading') { | ||
setIconAndPopup('disabled', tabId); | ||
} | ||
}); | ||
} | ||
} else { | ||
checkAndHandleRestrictedPageIfSo(tab); | ||
} | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay. I think this approach makes sense, but the placement of the inline comment is now pretty confusing. It's saying "don't do this in Chrome" inside of a callback we've added for Chrome, etc. I suggest we change it like so: // 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);
}
}); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made the above change in the latest commit |
||
|
||
chrome.runtime.onMessage.addListener((request, sender) => { | ||
if (sender.tab) { | ||
|
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.