This should make it easier to implement stuff that needs to scroll with the iframes by having one container that wraps the iframe and other things.

Alive, This patch adds a container as well as starts to use app_chrome.js in order to have 'rocketbar' controls. It is based on the titlebar branch from etienne's github repository, as the idea there is to have one titlebar/rocketbar per window (see bug 1042083). The strange part in the patch comes from the fact that I have both a view() and a combinedView() into app_chrome.js. The reason for that is because I would like to land without breaking the apps that relies on app_chrome today (facebook import in contacts and some e.me apps). The goal is to unify that and potentially only use the combinedView at the end, but there is a few UX questions there and what should be the exact behavior for those old apps - and I don't think this should block this work. The parts in app_window.js adds a wrapper around the iframe in order to enable "scrollgrab". The rest of the patch is a bunch of CSS changes - which has let me removed calibratedHeight! This patch likely breaks tons of tests in the current shape. I'm going to work on that but I think this patch could be reviewed as if. My goal would be to land that on friday if possible.

Comment on attachment 8461245 [details] [diff] [review] chromebar.patch Review of attachment 8461245 [details] [diff] [review]: ----------------------------------------------------------------- I guess you forgot git add something so made it hard to understand. Question: * Is every browser iframe expected to have scrollcrab attribute in its container? * What's the plan to merge the combinedView() and View()? I think the app title bar is something similar to what we have now in popupWindow? ::: apps/system/js/app_window.js @@ +716,5 @@ > new this.constructor.SUB_COMPONENTS[componentName](this); > } > } > + > + this.titleBar = new self.AppTitleBar(this); What is this? :/ @@ +1913,4 @@ > win.focus(); > }; > > + AppWindow.prototype._createBrowserContainer = Why not put this in AppWindow.prototype.view?

Well I saw the AppTitleBar in 1041859... My another question: How do you fix the popupWindow/activityWindow cases? If an app has an active popupWindow or activityWindow, what should be pushed down is the frontWindow instead of current appWindow's iframe? I am still uncertain what does scrollcrab do though. Is ALL iframe container with scrollcrab moving down at the same time? (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #2) > Comment on attachment 8461245 [details] [diff] [review] > chromebar.patch > > Review of attachment 8461245 [details] [diff] [review]: > ----------------------------------------------------------------- > > I guess you forgot git add something so made it hard to understand. > Question: > * Is every browser iframe expected to have scrollcrab attribute in its > container? > * What's the plan to merge the combinedView() and View()? I think the app > title bar is something similar to what we have now in popupWindow? > > ::: apps/system/js/app_window.js > @@ +716,5 @@ > > new this.constructor.SUB_COMPONENTS[componentName](this); > > } > > } > > + > > + this.titleBar = new self.AppTitleBar(this); > > What is this? :/ > > @@ +1913,4 @@ > > win.focus(); > > }; > > > > + AppWindow.prototype._createBrowserContainer = > > Why not put this in AppWindow.prototype.view?

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #3) > Well I saw the AppTitleBar in 1041859... bug 1041589 I mean.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #2) > * Is every browser iframe expected to have scrollcrab attribute in its > container? Most of them I would say. I have and hard time to think that lockscreen_window needs one, but the other may need one if they request some chrome. > * What's the plan to merge the combinedView() and View()? I think the app > title bar is something similar to what we have now in popupWindow? > The app titlebar is different as it lives in the statusbar. For view() and combinedView() I need to see on a case by case, looking at the UX spec how things can be merged. I think at the end we will completely get rid of view(), and so combinedView() would become view(). \ > > @@ +1913,4 @@ > > win.focus(); > > }; > > > > + AppWindow.prototype._createBrowserContainer = > > Why not put this in AppWindow.prototype.view? I wish I can :/ But the element.scrollgrab attribute needs to be set dynamically and it can't be as an attribute

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #3) > Well I saw the AppTitleBar in 1041859... > > My another question: > How do you fix the popupWindow/activityWindow cases? > > If an app has an active popupWindow or activityWindow, what should be pushed > down is the frontWindow instead of current appWindow's iframe? > Scrollgrab will not changed anything for that afaict.

Here is a version that should not break tests anymore with browser-container moved inside the view() method.

Same patch but the linter was complaining!

Comment on attachment 8461581 [details] [diff] [review] chromebar.patch Review of attachment 8461581 [details] [diff] [review]: ----------------------------------------------------------------- My first impression about this is coming from bug 1039519. It seems Cwiiis wanna to enable scrollcrab for every browser iframe. but this patch does not do that. * +1 for removing the calibrated height. My pain. * Not sure what's the plan for chrome-less appWindow. It seems scrollcrab is only for browserWindow or popupWindow right now. * PopupWindow is using appChrome(config.bar = true) * It might deserve to try generating the container in BrowserFrame and enable scrollcrab by browser config passed. Not necessary in the bug. * Please make sure everything is fine since we have lots of different windows right now. Sheet apps is your good friend to test. Keep watching for regressions. It seems popupWindow has no container, is this intentional? Does attentionWindow needs the container? Let's have a last review round because I think you need some test fixing stuff still. ::: apps/system/js/app_chrome.js @@ +17,5 @@ > */ > var AppChrome = function AppChrome(app) { > this.app = app; > this.instanceID = _id++; > + this.containerElement = app.browserContainer; This sounds incorrect... AFAIK the reason to have one more browser container is that we don't want to push the navigation UI down. If we render app chrome inside the container of the browser iframe and set scrollcrab, then we are just going back .... why not set scrollcrab on appWindow element? @@ +360,5 @@ > + this.app.config.chrome.bar) { > + view = this.combinedView(); > + } else { > + view = this.view(); > + } I highly suspect you need to change this.containerElement according to the config here. ::: apps/system/js/app_window.js @@ +726,2 @@ > > + this.browserContainer.scrollgrab = I don't understand this. ::: apps/system/js/base_ui.js @@ +22,5 @@ > * Overwrite `_registerEvents` to register event handler. > */ > BaseUI.prototype.render = function bu_render() { > this.publish('willrender'); > + this.containerElement.insertAdjacentHTML('afterbegin', this.view()); What's the change for? ::: apps/system/style/app_titlebar.css @@ +19,5 @@ > > .appWindow:not(.homescreen) .titlebar.expand { > background-size: 0 0, 100% 100%, 100% 100%; > transform: translateY(0); > + z-index: 1001; nit: I guess we need one more window_zindex.css...but could be done later. ::: apps/system/test/unit/activity_window_test.js @@ +32,5 @@ > > setup(function(done) { > + stubById = this.sinon.stub(document, 'getElementById', function(id) { > + var element = document.createElement('div'); > + if (id.indexOf('AppWindow') >= 0 || id.indexOf('activity-window') >= 0) { This looks ugly..and I don't think we have something like 'activity-window' but 'ActivityWindow_0'. Could you explain?

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #9) > Comment on attachment 8461581 [details] [diff] [review] > chromebar.patch > > Review of attachment 8461581 [details] [diff] [review]: > ----------------------------------------------------------------- > > My first impression about this is coming from bug 1039519. It seems Cwiiis > wanna to enable scrollcrab for every browser iframe. but this patch does not > do that. As you said it does not. The plan is to turn it on where it is needed right now. And the only place where it is needed is AppWindow with bar+navigation. In the near future 'navigation' will likely be removed. Then I hope to remove 'bar' as well. So all windows will end up using the exact same code path, with different elements activated in app_chrome based on config. > * Not sure what's the plan for chrome-less appWindow. It seems scrollcrab is > only for browserWindow or popupWindow right now. Ideally I would like to turn scrollgrab = true for every window by default. But doing so leads to weird results for now. So scrollgrab is only used for windows that have navigation+bar since those are the only one where browser-container is scrollable. > * PopupWindow is using appChrome(config.bar = true) Yep. It works fine. I pay attention to not regress app that use only 'bar' or 'navigation'. Those will be merged eventually but in the meantime I want to avoid regressions. > * It might deserve to try generating the container in BrowserFrame and > enable scrollcrab by browser config passed. Not necessary in the bug. I thought about it but I was uncertain about it. If you feel like it is the right thing to do let's do that in a followup as you suggest. > * Please make sure everything is fine since we have lots of different > windows right now. Sheet apps is your good friend to test. Keep watching for > regressions. > I will play a bit more with Sheets apps before landing. As you said there is a lot of windows now, and I feel like they will use a custom view more and more. Having to add something in each of them is not really helpful, because of this I think that you just convinced me that browser-container would be happy to live into browser_frame.js. > It seems popupWindow has no container, is this intentional? PopupWindow inherit its view from AppWindow no ? So it inherits browser-container. But I didn't activate the full chrome on them, because I didn't want to break them for now. > Does attentionWindow needs the container? > It would be simpler if every window have the container imho. > Let's have a last review round because I think you need some test fixing > stuff still. > > ::: apps/system/js/app_chrome.js > @@ +17,5 @@ > > */ > > var AppChrome = function AppChrome(app) { > > this.app = app; > > this.instanceID = _id++; > > + this.containerElement = app.browserContainer; > > This sounds incorrect... > AFAIK the reason to have one more browser container is that we don't want to > push the navigation UI down. If we render app chrome inside the container of > the browser iframe and set scrollcrab, then we are just going back .... why > not set scrollcrab on appWindow element? > Browser UI is the thing we want to scroll! > @@ +360,5 @@ > > + this.app.config.chrome.bar) { > > + view = this.combinedView(); > > + } else { > > + view = this.view(); > > + } > > I highly suspect you need to change this.containerElement according to the > config here. > Is it really needed if all windows have a container ? Sorry if I miss something obvious. > ::: apps/system/js/app_window.js > @@ +726,2 @@ > > > > + this.browserContainer.scrollgrab = > > I don't understand this. > I think that's because of your statement above about the chrome UI not beeing scrollable. I'm trying to the invert here :) > ::: apps/system/js/base_ui.js > @@ +22,5 @@ > > * Overwrite `_registerEvents` to register event handler. > > */ > > BaseUI.prototype.render = function bu_render() { > > this.publish('willrender'); > > + this.containerElement.insertAdjacentHTML('afterbegin', this.view()); > > What's the change for? > It generates some APZ failures in the sense that without this change scrollgrab does not work as expected :/ Likely one of the known platform issues that mess up the layer tree and prevent to construct the right chain. > ::: apps/system/style/app_titlebar.css > @@ +19,5 @@ > > > > .appWindow:not(.homescreen) .titlebar.expand { > > background-size: 0 0, 100% 100%, 100% 100%; > > transform: translateY(0); > > + z-index: 1001; > > nit: I guess we need one more window_zindex.css...but could be done later. > window_zindex.css would be nice. In fact one zindex file by stacking context may help. But yeah sounds definitively out of the scope of this bug. > ::: apps/system/test/unit/activity_window_test.js > @@ +32,5 @@ > > > > setup(function(done) { > > + stubById = this.sinon.stub(document, 'getElementById', function(id) { > > + var element = document.createElement('div'); > > + if (id.indexOf('AppWindow') >= 0 || id.indexOf('activity-window') >= 0) { > > This looks ugly..and I don't think we have something like 'activity-window' > but 'ActivityWindow_0'. Could you explain? https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/activity_window.js#L167 Seems like we used activity-window and not ActivityWindow :) Alive I'm not sure what you want me to fix in this patch exactly based on those comments ?

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #10) > (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #9) > > Comment on attachment 8461581 [details] [diff] [review] > > chromebar.patch > > > > Review of attachment 8461581 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > My first impression about this is coming from bug 1039519. It seems Cwiiis > > wanna to enable scrollcrab for every browser iframe. but this patch does not > > do that. > > As you said it does not. The plan is to turn it on where it is needed right > now. And the only place where it is needed is AppWindow with bar+navigation. Bar seems to be there for every window but hidden for some kind of window. So the problem is navigation, now navigation is on for manifest.chrome.navigation or it's a web page. If there's no plan to turn it on at a pure app window, let's do: this.app.browserContainer in appChrome after check bar&&navigation. > > In the near future 'navigation' will likely be removed. Then I hope to > remove 'bar' as well. So all windows will end up using the exact same code > path, with different elements activated in app_chrome based on config. That's what I mean above :) > > > * Not sure what's the plan for chrome-less appWindow. It seems scrollcrab is > > only for browserWindow or popupWindow right now. > > Ideally I would like to turn scrollgrab = true for every window by default. > But doing so leads to weird results for now. So scrollgrab is only used for > windows that have navigation+bar since those are the only one where > browser-container is scrollable. Makes sense. > > > * PopupWindow is using appChrome(config.bar = true) > > Yep. It works fine. I pay attention to not regress app that use only 'bar' > or 'navigation'. Those will be merged eventually but in the meantime I want > to avoid regressions. > > > * It might deserve to try generating the container in BrowserFrame and > > enable scrollcrab by browser config passed. Not necessary in the bug. > > I thought about it but I was uncertain about it. If you feel like it is the > right thing to do let's do that in a followup as you suggest. > > > * Please make sure everything is fine since we have lots of different > > windows right now. Sheet apps is your good friend to test. Keep watching for > > regressions. > > > > I will play a bit more with Sheets apps before landing. As you said there is > a lot of windows now, and I feel like they will use a custom view more and > more. Having to add something in each of them is not really helpful, because > of this I think that you just convinced me that browser-container would be > happy to live into browser_frame.js. Custom View() of each window is my intend but it seems a little pain right now.. The problem of each window is the construct flow of them are much different. Some are using BrowserFrame but some are not. I plan to clean these stuff after finishing attention window. Hope so. > > > It seems popupWindow has no container, is this intentional? > > PopupWindow inherit its view from AppWindow no ? So it inherits > browser-container. But I didn't activate the full chrome on them, because I > didn't want to break them for now. yes you are right. > > > Does attentionWindow needs the container? > > > > It would be simpler if every window have the container imho. > > > Let's have a last review round because I think you need some test fixing > > stuff still. > > > > ::: apps/system/js/app_chrome.js > > @@ +17,5 @@ > > > */ > > > var AppChrome = function AppChrome(app) { > > > this.app = app; > > > this.instanceID = _id++; > > > + this.containerElement = app.browserContainer; > > > > This sounds incorrect... > > AFAIK the reason to have one more browser container is that we don't want to > > push the navigation UI down. If we render app chrome inside the container of > > the browser iframe and set scrollcrab, then we are just going back .... why > > not set scrollcrab on appWindow element? > > > > Browser UI is the thing we want to scroll! > > > @@ +360,5 @@ > > > + this.app.config.chrome.bar) { > > > + view = this.combinedView(); > > > + } else { > > > + view = this.view(); > > > + } > > > > I highly suspect you need to change this.containerElement according to the > > config here. > > > > Is it really needed if all windows have a container ? Sorry if I miss > something obvious. > > > ::: apps/system/js/app_window.js > > @@ +726,2 @@ > > > > > > + this.browserContainer.scrollgrab = > > > > I don't understand this. > > > > I think that's because of your statement above about the chrome UI not > beeing scrollable. I'm trying to the invert here :) > > > ::: apps/system/js/base_ui.js > > @@ +22,5 @@ > > > * Overwrite `_registerEvents` to register event handler. > > > */ > > > BaseUI.prototype.render = function bu_render() { > > > this.publish('willrender'); > > > + this.containerElement.insertAdjacentHTML('afterbegin', this.view()); > > > > What's the change for? > > > > It generates some APZ failures in the sense that without this change > scrollgrab does not work as expected :/ Likely one of the known platform > issues that mess up the layer tree and prevent to construct the right chain. Do we have a bug for that? This is tricky and at least we need some comments to make sure people in the future doesn't regress... > > > ::: apps/system/style/app_titlebar.css > > @@ +19,5 @@ > > > > > > .appWindow:not(.homescreen) .titlebar.expand { > > > background-size: 0 0, 100% 100%, 100% 100%; > > > transform: translateY(0); > > > + z-index: 1001; > > > > nit: I guess we need one more window_zindex.css...but could be done later. > > > > window_zindex.css would be nice. In fact one zindex file by stacking context > may help. But yeah sounds definitively out of the scope of this bug. > > > ::: apps/system/test/unit/activity_window_test.js > > @@ +32,5 @@ > > > > > > setup(function(done) { > > > + stubById = this.sinon.stub(document, 'getElementById', function(id) { > > > + var element = document.createElement('div'); > > > + if (id.indexOf('AppWindow') >= 0 || id.indexOf('activity-window') >= 0) { > > > > This looks ugly..and I don't think we have something like 'activity-window' > > but 'ActivityWindow_0'. Could you explain? > > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/ > activity_window.js#L167 > > Seems like we used activity-window and not ActivityWindow :) > My bad...needs finetune in another bug. > > > Alive I'm not sure what you want me to fix in this patch exactly based on > those comments ? Not a reall fix, but what I want to know is the explanation above. Don't want to r+ because of trust or feature rush need. I am still a little confused about how scrollcrab work. Is there a bug or document for it?

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #10) > > This sounds incorrect... > > AFAIK the reason to have one more browser container is that we don't want to > > push the navigation UI down. If we render app chrome inside the container of > > the browser iframe and set scrollcrab, then we are just going back .... why > > not set scrollcrab on appWindow element? > > > > Browser UI is the thing we want to scroll! > Vivien, this is the last and main concern from me. Please convince me and you will have my r+. The alert/browsermenu/... stuff is also part of the so-called "browser UI", but they are rendered inside appWindow container element not the browser container element. so why don't we want to scroll all of the browser UI including the implemantation of browser API within appwindow element.....?

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #11) > > > > > > Alive I'm not sure what you want me to fix in this patch exactly based on > > those comments ? > > Not a real fix, > but what I want to know is the explanation above. Don't want to r+ because > of trust or feature rush need. > +1 for that! > I am still a little confused about how scrollcrab work. Is there a bug or > document for it? Sadly there is no real doc for it yet from what I know :( The best doc is found in the original bug history: bug 912666 A simple way to describe it would be: /*-----container scrollgrab = true --------*\ | | +------iframe mozbrowser------- ------------| | | | | | | \*------------------------------------------- Height of the container = Browser Chrome UI + Iframe height. Height of the iframe content = 10000px. With regular scroll: - If the user starts to scroll the iframe to reach the bottom of the iframe content, we will first scroll the iframe content. Once the user has reached the bottom of the iframe content, then we will scroll the container and show the chrome UI. - If the user start to scroll the iframe in the other direction, we will first scroll the iframe content again (leaving the chrome UI into view). Once the user has reached the top of the iframe content, the nwe will scroll the container and hide the chrome UI. With scrollgrab: scrollgrab tells the engine which element to scroll first. - If the user starts to scroll the iframe content to the reach the bottom of the iframe content, we will first scroll the container (and hide the chrome UI) and then the content until the user has reached the bottom of the iframe content. - If the user starts to scroll the iframe in the other direction, we will first scroll the container (and show the chrome UI) and then the content until the user has reached the top of the iframe content. This patch use scrollgrab for the scroll to bottom scenario and the scroll to top scenario. Ideally the behavior should be scrollgrab for the scroll to the bottom scenario, and regular scroll for the scroll to top scenario. I played with showing/removing scrollgrab on the fly to emulate that, but its racy. I think we would need a new 'scrollgrabontop' (poor naming) property in order to do that natively. But this is out of the scope of this bug :) Hope it helps.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #12) > (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, > needinfo? please) from comment #10) > > > This sounds incorrect... > > > AFAIK the reason to have one more browser container is that we don't want to > > > push the navigation UI down. If we render app chrome inside the container of > > > the browser iframe and set scrollcrab, then we are just going back .... why > > > not set scrollcrab on appWindow element? > > > > > > > Browser UI is the thing we want to scroll! > > > > Vivien, this is the last and main concern from me. Please convince me and > you will have my r+. > > The alert/browsermenu/... stuff is also part of the so-called "browser UI", > but they are rendered inside appWindow container element not the browser > container element. so why don't we want to scroll all of the browser UI > including the implemantation of browser API within appwindow element.....? That's a good question... The rocketbar plan is to have a minimize version of the chrome 'title'/input field in the statusbar when the user scroll the page to the bottom. So it means there is always something that should not be scrolled out of view and stay visible to the user. Since the 'statusbar' belongs for some parts to the app window now (background-color, icons, app title), we need to split the app window childs into a scrollable / non-scrollable part in order to keep this area in view all the time. Now about alert/contextmenu and so on, I don't have a real opinion if they should live in the non-scrollable part of the scrollable part (more a UX question). But it may seems a bit strange if the user is able to scroll part of the 'alert' UI out of view imho. Hope it convince you ;)

Comment on attachment 8461581 [details] [diff] [review] chromebar.patch Review of attachment 8461581 [details] [diff] [review]: ----------------------------------------------------------------- Convinced by the scrollcrab explanation and nonscrollable area. Thanks for your patience.

Let's move forward.

I don't think I've followed all of this conversation but I just wanted to note that having an outer scrollable scrollgrab div outside the iframe is definitely needed, but that all that needs to be above the iframe is an empty space. The browser chrome should be part of the expanded state of the app title bar (Rocketbar). We already have browser chrome at the bottom of the screen so moving it to the top of the screen doesn't really bring us any benefits as far as I can tell (I included more details in an email to Vivien and Etienne). I would prefer to move forward on adding browser navigation controls to the expanded state of the app title bar (Rocketbar). You can land this if you prefer, but for 2.1 I don't think there's any benefit of having the browser chrome inside the scrollable div as it doesn't move us towards the implementation we need. What we are aiming for for 2.1 is a time-based animation expanding/collapsing the Rocketbar (app title bar), based on scroll position of the outer scrollable div with scrollgrab, with an empty space for the expanded bar to fill.

(In reply to Ben Francis [:benfrancis] from comment #17) > I don't think I've followed all of this conversation but I just wanted to > note that having an outer scrollable scrollgrab div outside the iframe is > definitely needed, but that all that needs to be above the iframe is an > empty space. The browser chrome should be part of the expanded state of the > app title bar (Rocketbar). It can be moved in a separate bug. > > We already have browser chrome at the bottom of the screen so moving it to > the top of the screen doesn't really bring us any benefits as far as I can > tell (I included more details in an email to Vivien and Etienne). I would > prefer to move forward on adding browser navigation controls to the expanded > state of the app title bar (Rocketbar). > The necessary pieces of work to have the container height of 100% + empty space is big enough to not try to do everything in this patch. > You can land this if you prefer, but for 2.1 I don't think there's any > benefit of having the browser chrome inside the scrollable div as it doesn't > move us towards the implementation we need. What we are aiming for for 2.1 > is a time-based animation expanding/collapsing the Rocketbar (app title > bar), based on scroll position of the outer scrollable div with scrollgrab, > with an empty space for the expanded bar to fill. The animation is really a separate bug. This patch makes the scope of the animation bug much less invasive as all the iframe size calculation (which is painful) are already done.

Reverted in https://github.com/mozilla-b2g/gaia/commit/ebb824c7e4c1be271d68c773ac75ae0cfb79238d under suspicion of breaking JB emulator opt builds like this: https://tbpl.mozilla.org/php/getParsedLog.php?id=44609175&tree=B2g-Inbound Something from this b2g bumper bot push broke that build: https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=4aed8f0915cd so I reverted this patch and the patch from bug 1041455 to hopefully fix the build.

I re-triggered that job twice to see if it's an intermittent. Seems odd that this could cause that error.

Seeing how this is some infrastructure issue, and triggered on jobs prior to these landing, can we get these patches re-landed? https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=a418eea48be4

Go for it.

I guess this should be reopened if it was backed out? Vivien - do you want to reland this? I am fairly certain that the failure was not caused by this patch, and the backout was a mistake.

Revert the revert: https://github.com/mozilla-b2g/gaia/commit/33593d1e953de9626ae1ddd9f0e05556d28c023b

Looks like we started having perma-failures in 'Gij' after this landed, I'm going to back this out while we verify if that's the case. I wonder if this potentially crossed with something else while it was backed out. I recall us being green, so it's unfortunate we had an improper backout without a re-landing =/ If this happens again we should bring this up with sheriffs. https://tbpl.mozilla.org/php/getParsedLog.php?id=44678852&tree=Gaia-Try Revert the revert of the revert: https://github.com/mozilla-b2g/gaia/commit/a5f93c8ba175ae83fac6b7740a8414459fbd7a5b

Looks like one place is where this is failing is the CSS selector matching in integration tests. I'm seeing a bunch of these around the codebase - and they should probably be updated, '.inline-activity.active > iframe'. I'll try to catch you on Monday to see if you want some help with updating these tests. Hopefully we can find some less brittle selectors to use.

(In reply to Kevin Grandon :kgrandon from comment #28) > Looks like one place is where this is failing is the CSS selector matching > in integration tests. I'm seeing a bunch of these around the codebase - and > they should probably be updated, '.inline-activity.active > iframe'. > Thanks for digging into this. Just removing the '>' should be enough.

I fixed the test issues. Some of the tests were making assumptions about the DOM ordering. Which obviously broke with this patch. https://github.com/mozilla-b2g/gaia/commit/07ec460eaf99693e9a3968c0564d43156d823856

Just in case someone wants to backout it again - this is completely green: https://tbpl.mozilla.org/?tree=Gaia-Try&rev=85c2f948b5e8

So, it looks like the "Edge gestures" test is failing in Travis. It's not permared but nearly. And... yeah, it does not run on TBPL (I don't know why) that's why TBPL is completely green. See [1] for a failure, I relaunched the job triggered by your commit in [2] (it didn't previously finish because of an error). [1] https://travis-ci.org/mozilla-b2g/gaia/jobs/31045434 [2] https://travis-ci.org/mozilla-b2g/gaia/jobs/31036602 I'm fine with not backing it out if this proves to be a test-only failure, but this needs to be fixed. (and eventually enabled on TBPL too...) Can you please have a look?